diff --git a/src/Bravo.csproj b/src/Bravo.csproj index 58229c26..38b471e6 100644 --- a/src/Bravo.csproj +++ b/src/Bravo.csproj @@ -55,6 +55,7 @@ + diff --git a/src/Host/BravoApplicationInstance.cs b/src/Host/BravoApplicationInstance.cs index f9b1bb18..2d882f25 100644 --- a/src/Host/BravoApplicationInstance.cs +++ b/src/Host/BravoApplicationInstance.cs @@ -1,6 +1,6 @@ using System; using System.Diagnostics; -using System.Text; +using System.IO; using System.Text.Json; using Sqlbi.Bravo.Infrastructure.Configuration.Settings; using Sqlbi.Bravo.Infrastructure.Helpers; @@ -13,7 +13,7 @@ namespace Sqlbi.Bravo.Host; /// /// Defines the contract for a single-instance application. /// -internal interface IBravoApplicationInstance : IInstanceActivationEvents, IDisposable +internal interface IBravoApplicationInstance : IInstanceEvents, IDisposable { /// /// Gets a value indicating whether this process is the primary instance, the one that runs the application. @@ -71,45 +71,41 @@ private void OnActivated(object? sender, SingleInstanceActivatedEventArgs e) var startupMessage = default(AppInstanceStartupMessage?); try { - var json = Encoding.UTF8.GetString(e.Payload); - startupMessage = JsonSerializer.Deserialize(json); + startupMessage = JsonSerializer.Deserialize(e.Payload); } catch (JsonException) { // An unreadable payload still activates the window, with no document to open. } - // ACCEPTED LIMITATION — requests arriving while this instance is still starting up are - // dropped here, on purpose. The pipe answers from the moment the process starts, but the - // subscribers appear later: AppWindow attaches the bring-to-front handler in OnLoad, and the - // one that forwards the startup message only in OnWebViewDOMContentLoaded, seconds later. - // So a request that lands before the window exists is lost entirely, and one that lands - // between the window and a loaded WebView brings Bravo to the front but does not open what - // was asked for. The secondary instance is told the payload was delivered either way, so - // nothing records the loss. + // Known limitation: requests arriving while this instance is still starting up are dropped + // here. The pipe answers from the moment the process starts, but the subscribers appear + // later: AppWindow attaches the bring-to-front handler in OnLoad, and the one that forwards + // the startup message only in OnWebViewDOMContentLoaded, seconds later. A request that lands + // before the window exists is lost, and one that lands between the window and a loaded + // WebView brings Bravo to the front but does not open what was asked for. The secondary + // instance is told the payload was delivered either way. // TODO: Consider buffering the requests and replaying them once the UI is ready. ActivationRequested?.Invoke(this, new InstanceActivationRequestedEventArgs(startupMessage)); } - private void OnError(object? sender, SingleInstanceErrorEventArgs e) => Report(e.Exception); + private void OnError(object? sender, ErrorEventArgs e) => Report(e.GetException()); /// public void RedirectActivationToPrimary() { var startupSettings = StartupSettings.CreateFromCommandLineArguments(); var startupMessage = AppInstanceStartupMessage.CreateFrom(startupSettings); - var json = JsonSerializer.Serialize(startupMessage); - var payload = Encoding.UTF8.GetBytes(json); + var payload = JsonSerializer.SerializeToUtf8Bytes(startupMessage); var result = SingleInstanceClient.Send(_options, payload); - if (!result.IsDelivered && result.Exception is not null) + if (!result.IsDelivered) { - // The primary instance cannot be reached (for example, an elevated "Run as administrator" - // launch against a non-elevated instance), activation fails silently. This can happen because - // PipeOptions.CurrentUserOnly compares WindowsIdentity.Owner, which differs when elevation changes - // the identity to BUILTIN\Administrators. The failure is recorded in telemetry and the event log - // (BravoApplicationInstance.RedirectActivationToPrimary), but no user-facing error is shown. - + // The primary instance cannot be reached and activation fails silently. One known cause is + // a launch whose elevation differs from the primary's: PipeOptions.CurrentUserOnly builds + // and checks the pipe ACL on WindowsIdentity.Owner, which elevation changes to + // BUILTIN\Administrators, while the pipe name is keyed on WindowsIdentity.User + // (https://github.com/dotnet/runtime/issues/123903, open, milestone .NET 12). // TODO: Consider surfacing activation failures to the user. Report(result.Exception); } diff --git a/src/Host/BravoServiceCollectionExtensions.cs b/src/Host/BravoServiceCollectionExtensions.cs index bda4a302..14913186 100644 --- a/src/Host/BravoServiceCollectionExtensions.cs +++ b/src/Host/BravoServiceCollectionExtensions.cs @@ -23,7 +23,7 @@ public static IServiceCollection AddBravoInitializationServices(this IServiceCol { // The single instance goes in as its activation events only: IsPrimary and the activation // redirection belong to the entry point and stay out of the container. - services.AddSingleton(context.Instance); + services.AddSingleton(context.Instance); services.AddSingleton(new PolicyServiceFactory().Create()); services.AddSingleton(TelemetryService.Instance); diff --git a/src/Host/IInstanceActivationEvents.cs b/src/Host/IInstanceEvents.cs similarity index 69% rename from src/Host/IInstanceActivationEvents.cs rename to src/Host/IInstanceEvents.cs index 8cb6504e..b9768dec 100644 --- a/src/Host/IInstanceActivationEvents.cs +++ b/src/Host/IInstanceEvents.cs @@ -2,10 +2,7 @@ namespace Sqlbi.Bravo.Host; -/// -/// Defines the activation events this process instance raises for interested subscribers. -/// -internal interface IInstanceActivationEvents +internal interface IInstanceEvents { /// /// Occurs on the primary instance when another instance asks it to come forward. diff --git a/src/Host/InstanceActivationRequestedEventArgs.cs b/src/Host/InstanceActivationRequestedEventArgs.cs index cfbff5e1..4d5b6829 100644 --- a/src/Host/InstanceActivationRequestedEventArgs.cs +++ b/src/Host/InstanceActivationRequestedEventArgs.cs @@ -4,7 +4,7 @@ namespace Sqlbi.Bravo.Host; /// -/// Provides data for the event. +/// Provides data for the event. /// internal class InstanceActivationRequestedEventArgs(AppInstanceStartupMessage? startupMessage) : EventArgs { diff --git a/src/Host/InstancePipeName.cs b/src/Host/InstancePipeName.cs index 7c7db824..0f6dcd9c 100644 --- a/src/Host/InstancePipeName.cs +++ b/src/Host/InstancePipeName.cs @@ -9,19 +9,17 @@ namespace Sqlbi.Bravo.Host; /// /// /// -/// THE RULE: one instance per Windows session and per account, across elevation levels, and -/// regardless of how Bravo was installed — Portable, both MSI variants and the MSIX/Store package -/// are the same application to the user, and must not run side by side just because one of them -/// happens to come from a different install mechanism. -/// -/// -/// The format is a frozen contract: changing it makes a new build unable to see an instance started -/// by an older one. It is kept as a pure function so that it can be pinned by tests without touching -/// the registry, the process or the current Windows identity. +/// One instance per Windows session and per account, across elevation levels and regardless of how +/// Bravo was installed: Portable, both MSI variants and the MSIX/Store package are the same +/// application to the user and must not run side by side. /// /// internal static class InstancePipeName { + // Hardcoded constants (stable across releases) + private const string ApplicationName = "Bravo"; + private const string ScopeId = "8D4D9F1D39F94C7789D84729480D8198"; + /// /// Builds the name for the current user and session. /// @@ -29,33 +27,26 @@ public static string Create() { using var identity = WindowsIdentity.GetCurrent(); - // User, not the token's default object owner. A "run as" with a different user gets its own - // instance — that account has its own %LOCALAPPDATA%, MSAL cache and HKCU, so nothing is - // shared with it. A "run as administrator" must not get its own instance: UAC elevation keeps - // the same Windows account, so usersettings.json, the WebView2 user data folder and - // .msalcache are the same files, and they need a single writer. Owner would get this backwards - // — elevation changes it to BUILTIN\Administrators, while User stays stable across elevation. + // User, not the token owner. A "run as" with a different user gets its own instance: that + // account has its own %LOCALAPPDATA%, MSAL cache and HKCU, so nothing is shared. A "run as + // administrator" must not: UAC elevation keeps the same account, so usersettings.json, the + // WebView2 user data folder and .msalcache are the same files and need a single writer. + // Elevation changes the owner to BUILTIN\Administrators, while User stays the same. var userSid = identity.User?.Value ?? throw new InvalidOperationException("The current Windows identity has no user SID."); return Create(AppEnvironment.SessionId, userSid); } + /// + /// Builds the name for the given session and account. + /// /// - /// Identifies the Windows session. Every Terminal Services session needs its own window and its - /// own Bravo instance, so a different session is always a different instance. + /// The Windows session. Every Terminal Services session has its own window, so it is a different instance. /// /// - /// Identifies the Windows account and must remain stable across UAC elevation — see - /// for why this is User rather than the token owner. + /// The Windows account. Must be stable across UAC elevation; see . /// public static string Create(int sessionId, string userSid) - { - // Both constants below are hardcoded; they are part of the frozen - // contract that makes the instance name stable across releases. - const string ApplicationName = "Bravo"; - const string ScopeId = "8D4D9F1D39F94C7789D84729480D8198"; - - return $"{ApplicationName}.{ScopeId}.{sessionId}.{userSid}"; - } + => $"{ApplicationName}.{ScopeId}.{sessionId}.{userSid}"; } diff --git a/src/Infrastructure/AppWindow.cs b/src/Infrastructure/AppWindow.cs index 448de2a2..93d105b4 100644 --- a/src/Infrastructure/AppWindow.cs +++ b/src/Infrastructure/AppWindow.cs @@ -41,7 +41,7 @@ internal partial class AppWindow : Form };"; public static SynchronizationContext? UISynchronizationContext { get; set; } - private readonly IInstanceActivationEvents _instanceActivationEvents; + private readonly IInstanceEvents _instanceEvents; private readonly IServerAddressProvider _serverAddressProvider; private readonly IOptions _startupSettingsOptionsAccessor; private readonly WebView2ProxyAuthHandler _proxyAuthHandler; @@ -49,12 +49,12 @@ internal partial class AppWindow : Form private readonly IPolicyService _policyService; public AppWindow( - IInstanceActivationEvents instanceActivationEvents, + IInstanceEvents instanceEvents, IServerAddressProvider serverAddressProvider, IOptions startupSettingsOptionsAccessor, IPolicyService policyService) { - _instanceActivationEvents = instanceActivationEvents; + _instanceEvents = instanceEvents; _serverAddressProvider = serverAddressProvider; _startupSettingsOptionsAccessor = startupSettingsOptionsAccessor; _policyService = policyService; @@ -183,7 +183,7 @@ private void OnFormLoad(object? sender, EventArgs e) CenterToScreen(); - _instanceActivationEvents.ActivationRequested += OnActivationRequestedRestoreWindowToForeground; + _instanceEvents.ActivationRequested += OnActivationRequestedRestoreWindowToForeground; if (AppEnvironment.IsDiagnosticLevelVerbose) { @@ -194,8 +194,8 @@ private void OnFormLoad(object? sender, EventArgs e) private void OnFormClosed(object? sender, FormClosedEventArgs e) { - _instanceActivationEvents.ActivationRequested -= OnActivationRequestedRestoreWindowToForeground; - _instanceActivationEvents.ActivationRequested -= OnActivationRequestedSendStartupWebMessage; + _instanceEvents.ActivationRequested -= OnActivationRequestedRestoreWindowToForeground; + _instanceEvents.ActivationRequested -= OnActivationRequestedSendStartupWebMessage; } private void OnWebViewDOMContentLoaded(object? sender, CoreWebView2DOMContentLoadedEventArgs e) @@ -208,7 +208,7 @@ private void OnWebViewDOMContentLoaded(object? sender, CoreWebView2DOMContentLoa BackgroundImage = null; SendAppStartupWebMessage(); - _instanceActivationEvents.ActivationRequested += OnActivationRequestedSendStartupWebMessage; + _instanceEvents.ActivationRequested += OnActivationRequestedSendStartupWebMessage; } } diff --git a/src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs b/src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs index 288ac5ea..ff5b581c 100644 --- a/src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs +++ b/src/Infrastructure/SingleInstance/SingleInstanceActivatedEventArgs.cs @@ -3,13 +3,12 @@ namespace Sqlbi.Bravo.Infrastructure.SingleInstance; /// -/// Carries the raw payload sent by a secondary instance. The component is deliberately unaware of -/// how the payload is encoded: interpreting it is the composition layer's responsibility. +/// Carries the payload sent by a secondary instance. /// +/// +/// The payload is not interpreted by the server. It is never empty. +/// internal sealed class SingleInstanceActivatedEventArgs(byte[] payload) : EventArgs { - /// - /// The bytes received from the secondary instance. Never empty. - /// public byte[] Payload { get; } = payload; } diff --git a/src/Infrastructure/SingleInstance/SingleInstanceClient.cs b/src/Infrastructure/SingleInstance/SingleInstanceClient.cs index d515b4e8..646a2a99 100644 --- a/src/Infrastructure/SingleInstance/SingleInstanceClient.cs +++ b/src/Infrastructure/SingleInstance/SingleInstanceClient.cs @@ -14,11 +14,14 @@ internal static class SingleInstanceClient /// Sends to the owning instance. /// /// - /// The connection is opened with , which makes the - /// client verify that the pipe is owned by the current user before writing to it. + /// makes the client verify that the pipe is owned by + /// the current user before writing to it. The write completes when the owner has read the + /// payload, so a delivered payload has reached the owner; an owner that is alive but not + /// reading blocks the caller, with no timeout. /// public static SingleInstanceSendResult Send(SingleInstanceOptions options, byte[] payload) { + ArgumentOutOfRangeException.ThrowIfZero(payload.Length); ArgumentOutOfRangeException.ThrowIfGreaterThan(payload.Length, options.MaxPayloadBytes); using var pipeClient = new NamedPipeClientStream( @@ -39,9 +42,8 @@ public static SingleInstanceSendResult Send(SingleInstanceOptions options, byte[ try { pipeClient.Write(payload, offset: 0, payload.Length); - pipeClient.Flush(); } - catch (Exception ex) when (ex is IOException or ObjectDisposedException or InvalidOperationException) + catch (IOException ex) { return SingleInstanceSendResult.Failed(ex); } diff --git a/src/Infrastructure/SingleInstance/SingleInstanceErrorEventArgs.cs b/src/Infrastructure/SingleInstance/SingleInstanceErrorEventArgs.cs deleted file mode 100644 index e8e3dac8..00000000 --- a/src/Infrastructure/SingleInstance/SingleInstanceErrorEventArgs.cs +++ /dev/null @@ -1,13 +0,0 @@ -using System; - -namespace Sqlbi.Bravo.Infrastructure.SingleInstance; - -/// -/// Reports a failure that happened on the listener loop, where there is no caller to return it to. -/// Modelled on : the component owns no logging -/// dependency, so the host decides what to do with it. -/// -internal sealed class SingleInstanceErrorEventArgs(Exception exception) : EventArgs -{ - public Exception Exception { get; } = exception; -} diff --git a/src/Infrastructure/SingleInstance/SingleInstanceOptions.cs b/src/Infrastructure/SingleInstance/SingleInstanceOptions.cs index dc33f6db..b927f755 100644 --- a/src/Infrastructure/SingleInstance/SingleInstanceOptions.cs +++ b/src/Infrastructure/SingleInstance/SingleInstanceOptions.cs @@ -3,38 +3,41 @@ namespace Sqlbi.Bravo.Infrastructure.SingleInstance; /// -/// Configuration shared by the two sides of the single-instance protocol: the owner -/// () and any secondary instance (). -/// Both must be configured with the same . +/// Configuration shared by and . /// +/// +/// Both sides must use the same . +/// internal sealed record SingleInstanceOptions { /// - /// Name of the named pipe that both identifies the application and arbitrates ownership of the - /// running instance. It must be stable across releases, and unique per user and per session: - /// everything sharing this name is considered the same application instance. + /// Name of the named pipe that identifies the application instance. /// + /// + /// Everything sharing this name is the same application instance, so it must be stable across + /// releases and unique per user and per session. + /// public required string PipeName { get; init; } /// - /// How long a secondary instance waits for the owner to accept its connection before giving up. + /// How long a secondary instance waits for the owner to accept its connection. /// public TimeSpan ConnectTimeout { get; init; } = TimeSpan.FromSeconds(5); /// - /// How long the owner waits, once a client has connected, for that client to finish sending its - /// payload. The counterpart of on the owner's side: a legitimate - /// client writes its whole payload in one call right after connecting, so this bounds how long a - /// stalled or malfunctioning client can hold the pipe's only server instance, which would - /// otherwise leave the owner unreachable to every later instance while it stays alive. + /// How long the owner waits for a connected client to finish sending its payload. /// + /// + /// A client that connects and never finishes would otherwise hold the pipe's only server + /// instance, leaving the owner unreachable while it stays alive. + /// public TimeSpan ReadTimeout { get; init; } = TimeSpan.FromSeconds(5); /// - /// Upper bound, in bytes, for a single activation payload. The limit is checked after each read, - /// so a rejected payload is buffered up to one read block past it and never more, keeping a - /// malfunctioning or hostile client from growing the owner's memory. The rejection is reported - /// through . + /// Upper bound, in bytes, for a single activation payload. /// + /// + /// A larger payload is rejected and reported through . + /// public int MaxPayloadBytes { get; init; } = 64 * 1024; } diff --git a/src/Infrastructure/SingleInstance/SingleInstanceSendResult.cs b/src/Infrastructure/SingleInstance/SingleInstanceSendResult.cs index 69e66b67..e6be8e3d 100644 --- a/src/Infrastructure/SingleInstance/SingleInstanceSendResult.cs +++ b/src/Infrastructure/SingleInstance/SingleInstanceSendResult.cs @@ -1,4 +1,5 @@ using System; +using System.Diagnostics.CodeAnalysis; namespace Sqlbi.Bravo.Infrastructure.SingleInstance; @@ -13,8 +14,7 @@ internal enum SingleInstanceSendStatus Delivered = 0, /// - /// No owner accepted the connection within the configured timeout. Either the owner is gone, or - /// it is alive but not listening — which the caller may want to surface rather than ignore. + /// No owner accepted the connection within the configured timeout, or the owner refused it. /// OwnerUnavailable = 1, @@ -25,15 +25,29 @@ internal enum SingleInstanceSendStatus } /// -/// Result of a send. Failing to reach the owner is an expected -/// runtime condition, not an exceptional one, so it is returned rather than thrown. +/// Result of a send. /// -internal readonly record struct SingleInstanceSendResult(SingleInstanceSendStatus Status, Exception? Exception) +/// +/// Failing to reach the owner is an expected runtime condition, so it is returned rather than +/// thrown. A failed result always carries the exception that caused it. +/// +internal readonly record struct SingleInstanceSendResult { + private SingleInstanceSendResult(SingleInstanceSendStatus status, Exception? exception) + { + Status = status; + Exception = exception; + } + + public SingleInstanceSendStatus Status { get; } + + public Exception? Exception { get; } + + [MemberNotNullWhen(false, nameof(Exception))] public bool IsDelivered => Status == SingleInstanceSendStatus.Delivered; public static SingleInstanceSendResult Delivered() - => new(SingleInstanceSendStatus.Delivered, Exception: null); + => new(SingleInstanceSendStatus.Delivered, exception: null); public static SingleInstanceSendResult OwnerUnavailable(Exception exception) => new(SingleInstanceSendStatus.OwnerUnavailable, exception); diff --git a/src/Infrastructure/SingleInstance/SingleInstanceServer.cs b/src/Infrastructure/SingleInstance/SingleInstanceServer.cs index d1e9c4e2..680b8559 100644 --- a/src/Infrastructure/SingleInstance/SingleInstanceServer.cs +++ b/src/Infrastructure/SingleInstance/SingleInstanceServer.cs @@ -1,9 +1,12 @@ using System; +using System.Buffers; +using System.ComponentModel; using System.Diagnostics.CodeAnalysis; using System.IO; using System.IO.Pipes; using System.Threading; using System.Threading.Tasks; +using Sqlbi.Bravo.Infrastructure.Windows.Interop; namespace Sqlbi.Bravo.Infrastructure.SingleInstance; @@ -14,26 +17,27 @@ namespace Sqlbi.Bravo.Infrastructure.SingleInstance; /// /// /// Ownership is arbitrated by the named pipe itself, which is created allowing a single server -/// instance: only one process at a time can hold the name. There is deliberately no separate mutex, -/// so there is no second piece of state that can disagree with the listener. If this process stops -/// listening for any reason, the name is released and the next process to start becomes the owner, -/// instead of every later instance failing to reach an owner that no longer answers. +/// instance: only one process at a time can hold the name. There is no separate mutex, so there is +/// no second piece of state that can disagree with the listener. If this process stops listening +/// for any reason, the name is released and the next process to start becomes the owner. /// /// -/// The pipe is disconnected — not recreated — between connections: recreating it would release the +/// The pipe is disconnected, not recreated, between connections: recreating it would release the /// name for an instant, during which another process could claim ownership. /// /// internal sealed class SingleInstanceServer : IDisposable { - private static readonly TimeSpan s_errorBackoff = TimeSpan.FromMilliseconds(250); + private const int ErrorPipeBusy = unchecked((int)0x800700E7); + + private static readonly TimeSpan s_acceptFailureBackoff = TimeSpan.FromMilliseconds(250); private static readonly TimeSpan s_shutdownTimeout = TimeSpan.FromSeconds(1); private readonly SingleInstanceOptions _options; private readonly NamedPipeServerStream _pipeServer; private readonly CancellationTokenSource _cancellation; + private readonly Task _listener; - private Task? _listener; private bool _disposed; private SingleInstanceServer(SingleInstanceOptions options, NamedPipeServerStream pipeServer) @@ -41,21 +45,29 @@ private SingleInstanceServer(SingleInstanceOptions options, NamedPipeServerStrea _options = options; _pipeServer = pipeServer; _cancellation = new CancellationTokenSource(); + // Started on the thread pool so that the loop runs with no SynchronizationContext, whatever + // thread called TryStart: every continuation stays on the pool, and the blocking wait in + // Dispose cannot deadlock against the caller's context. + _listener = Task.Run(() => ListenAsync(_cancellation.Token)); } /// - /// Raised when a secondary instance sends an activation payload. Raised on a thread pool thread, - /// never on the listener loop, so a subscriber is free to block: subscribers that need a specific - /// thread — a UI thread, typically — marshal by themselves. + /// Raised when a secondary instance sends an activation payload. /// + /// + /// Raised on a thread pool thread, not on the listener loop: a blocking handler does not stop + /// the listener. Handlers for different payloads may run concurrently and in any order. + /// public event EventHandler? Activated; /// - /// Raised when a connection fails, a client does not finish sending within - /// , or a subscriber of - /// throws. The loop keeps running: ownership is not given up because of a single bad connection. + /// Raised when a connection fails, a client sends no valid payload within + /// , or an handler throws. /// - public event EventHandler? Error; + /// + /// The listener continues after the event. Handlers may be invoked concurrently. + /// + public event EventHandler? Error; /// /// Attempts to take ownership of the application instance identified by @@ -63,14 +75,17 @@ private SingleInstanceServer(SingleInstanceOptions options, NamedPipeServerStrea /// /// /// if ownership was acquired by this process; if - /// another process already owns it, in which case the caller should notify the owner through - /// and exit. + /// another process already owns it. /// public static bool TryStart(SingleInstanceOptions options, [NotNullWhen(true)] out SingleInstanceServer? server) { NamedPipeServerStream pipeServer; try { + // The default zero-sized buffer is deliberate: a client's Write then blocks until this + // process reads, which can only happen after the connection is accepted. With a buffer, + // a client that writes and closes before ConnectNamedPipe completes makes the accept fail + // with ERROR_NO_DATA, and the payload in the buffer is lost. pipeServer = new NamedPipeServerStream( options.PipeName, PipeDirection.In, @@ -78,122 +93,118 @@ public static bool TryStart(SingleInstanceOptions options, [NotNullWhen(true)] o PipeTransmissionMode.Byte, PipeOptions.Asynchronous | PipeOptions.CurrentUserOnly); } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + catch (Exception ex) when (ex is UnauthorizedAccessException || (ex is IOException && ex.HResult == ErrorPipeBusy)) { - // The name is already taken: another process owns this application instance. + // ERROR_PIPE_BUSY: the single allowed instance is held by another process. Access denied: + // it is held by a process whose pipe ACL excludes this one. Any other failure propagates. server = null; return false; } - var instance = new SingleInstanceServer(options, pipeServer); - instance._listener = Task.Run(() => instance.ListenAsync(instance._cancellation.Token)); - - server = instance; + server = new SingleInstanceServer(options, pipeServer); return true; } /// - /// Serves one connection at a time until shutdown. A bad connection degrades this loop, it never - /// ends it: giving up would release the pipe name and hand ownership to the next process. + /// Serves one connection at a time until shutdown. /// + /// + /// A failed connection is reported and the loop continues: ending the loop would release the + /// pipe name. Only a failure to accept is paced, since a repeatedly failing pipe would otherwise + /// spin. A client that fails after connecting is not paced: the pipe is intact and the next + /// client may already be waiting for the instance. + /// private async Task ListenAsync(CancellationToken cancellationToken) { - while (!cancellationToken.IsCancellationRequested) + try { - try + while (true) { - var payload = await AcceptAndReadAsync(cancellationToken).ConfigureAwait(false); - if (payload is not null) - Dispatch(payload); - } - catch (Exception ex) when (IsShutdown(ex, cancellationToken)) - { - break; - } - catch (Exception ex) - { - RaiseError(ex); + try + { + await _pipeServer.WaitForConnectionAsync(cancellationToken); + } + catch (Exception ex) when (!IsShutdown(ex, cancellationToken)) + { + DisconnectAfterFailedAccept(); + RaiseError(ex); + await Task.Delay(s_acceptFailureBackoff, cancellationToken); + continue; + } - if (!await TryBackOffAsync(cancellationToken).ConfigureAwait(false)) - break; + try + { + Dispatch(await ReadPayloadAsync(cancellationToken)); + } + catch (Exception ex) when (!IsShutdown(ex, cancellationToken)) + { + RaiseError(ex); + } + finally + { + Disconnect(); + } } } + catch (Exception ex) when (IsShutdown(ex, cancellationToken)) + { + } } /// - /// Tells a shutdown apart from a connection that went wrong. The two arrive as the same exception - /// types, and only the caller's token says which happened: a cancelled read is the ReadTimeout - /// firing, unless shutdown was requested. + /// Returns whether was caused by shutdown rather than by a failed + /// connection. /// - private static bool IsShutdown(Exception exception, CancellationToken cancellationToken) => exception switch - { - // The pipe was disposed underneath the loop, which only Dispose does. - ObjectDisposedException => true, - OperationCanceledException => cancellationToken.IsCancellationRequested, - _ => false, - }; + /// + /// Disposing the pipe under a pending operation surfaces as an , an + /// or an depending + /// on timing, so the shutdown token decides, not the exception type. + /// + private static bool IsShutdown(Exception exception, CancellationToken cancellationToken) + => cancellationToken.IsCancellationRequested || exception is ObjectDisposedException; /// - /// Accepts one connection and reads its payload, always releasing the pipe afterwards. Returns - /// when the client sent nothing usable. + /// Reads the payload of the connected client, up to the client closing its end of the pipe. /// - private async Task AcceptAndReadAsync(CancellationToken cancellationToken) + /// The client did not finish within . + /// The payload is empty or exceeds . + private async Task ReadPayloadAsync(CancellationToken cancellationToken) { + // Waiting for a connection has no deadline; once connected a deadline applies, otherwise a + // client that never finishes sending would hold the pipe's only server instance. + using var readCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + readCancellation.CancelAfter(_options.ReadTimeout); + + var limit = _options.MaxPayloadBytes; + var buffer = ArrayPool.Shared.Rent(limit + 1); try { - // Waiting for a connection has no deadline: an idle pipe with nobody connected is the - // normal state, not a problem. - await _pipeServer.WaitForConnectionAsync(cancellationToken).ConfigureAwait(false); + var length = 0; + while (length <= limit) + { + var count = await _pipeServer.ReadAsync(buffer.AsMemory(length, buffer.Length - length), readCancellation.Token); + if (count == 0) + break; - // Once connected a deadline applies, otherwise a client that connects and never finishes - // sending would hold the pipe's only server instance forever — see ReadTimeout. - using var readCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - readCancellation.CancelAfter(_options.ReadTimeout); + length += count; + } - return await ReadPayloadAsync(readCancellation.Token).ConfigureAwait(false); - } - finally - { - Disconnect(); - } - } + if (length > limit) + throw new InvalidDataException($"The payload exceeds {limit} bytes."); - /// - /// Paces the loop after a failure, so that a pipe or a client failing immediately and repeatedly - /// leaves the listener degraded rather than spinning at full speed. - /// - /// if shutdown was requested while waiting. - private static async Task TryBackOffAsync(CancellationToken cancellationToken) - { - try - { - await Task.Delay(s_errorBackoff, cancellationToken).ConfigureAwait(false); - return true; + if (length == 0) + throw new InvalidDataException("The payload is empty."); + + return buffer.AsSpan(0, length).ToArray(); } - catch (OperationCanceledException) + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) { - return false; + throw new TimeoutException($"The client did not finish sending within {_options.ReadTimeout}."); } - } - - private async Task ReadPayloadAsync(CancellationToken cancellationToken) - { - using var buffer = new MemoryStream(); - var chunk = new byte[4096]; - - int count; - while ((count = await _pipeServer.ReadAsync(chunk, cancellationToken).ConfigureAwait(false)) > 0) + finally { - buffer.Write(chunk, 0, count); - - // Thrown rather than returned as null so that it travels the same path as any other bad - // connection: reported through Error, then paced. Returning null would have made an - // oversized payload indistinguishable from an empty one and dropped it silently. - if (buffer.Length > _options.MaxPayloadBytes) - throw new InvalidDataException($"Payload exceeds {_options.MaxPayloadBytes} bytes."); + ArrayPool.Shared.Return(buffer); } - - return buffer.Length == 0 ? null : buffer.ToArray(); } private void Dispatch(byte[] payload) @@ -219,11 +230,11 @@ private void RaiseError(Exception exception) { try { - Error?.Invoke(this, new SingleInstanceErrorEventArgs(exception)); + Error?.Invoke(this, new ErrorEventArgs(exception)); } catch { - // A failing error handler must not take down the listener loop. + // An exception from an Error handler does not end the listener loop. } } @@ -231,15 +242,30 @@ private void Disconnect() { try { - // Unconditionally, and never guarded by IsConnected: when the client closes first the - // property is already false while the pipe instance is still in a connected state, and - // skipping the call makes every later WaitForConnectionAsync fail with - // InvalidOperationException — the owner stops answering while still holding the name. + // Not guarded by IsConnected: when the client closes first, IsConnected is already false + // while the instance is still connected, and a skipped Disconnect makes every later + // WaitForConnectionAsync throw InvalidOperationException. _pipeServer.Disconnect(); } catch (Exception ex) when (ex is IOException or ObjectDisposedException or InvalidOperationException) { - // Not connected, or already gone: nothing to release. + // Not connected, or already disposed: nothing to release. + } + } + + private void DisconnectAfterFailedAccept() + { + try + { + // A failed WaitForConnectionAsync leaves the stream waiting to connect, a state in which + // NamedPipeServerStream.Disconnect throws without disconnecting. When the client closed + // during the accept, the instance stays closing and every later accept fails until it is + // disconnected, so the owner keeps the name but answers nobody. + PipeInterop.DisconnectNamedPipe(_pipeServer.SafePipeHandle); + } + catch (Exception ex) when (ex is Win32Exception or ObjectDisposedException or InvalidOperationException) + { + // Not connected, or already disposed: nothing to release. } } @@ -251,17 +277,10 @@ public void Dispose() _disposed = true; _cancellation.Cancel(); - // Unblocks a WaitForConnectionAsync that is not observing cancellation yet. _pipeServer.Dispose(); - try - { - _listener?.Wait(s_shutdownTimeout); - } - catch (AggregateException) - { - // The loop faulted on its way out; there is nothing left to report at this point. - } + // ListenAsync catches every exception, so this only returns or times out. + _listener.Wait(s_shutdownTimeout); _cancellation.Dispose(); } diff --git a/src/Infrastructure/Windows/Interop/PipeInterop.cs b/src/Infrastructure/Windows/Interop/PipeInterop.cs new file mode 100644 index 00000000..52d1a068 --- /dev/null +++ b/src/Infrastructure/Windows/Interop/PipeInterop.cs @@ -0,0 +1,18 @@ +using System.ComponentModel; +using Microsoft.Win32.SafeHandles; +using Windows.Win32; + +namespace Sqlbi.Bravo.Infrastructure.Windows.Interop; + +internal static class PipeInterop +{ + /// + /// Disconnects the server end of a named pipe instance from its client. + /// + /// The pipe instance could not be disconnected. + public static void DisconnectNamedPipe(SafePipeHandle pipeHandle) + { + if (!PInvoke.DisconnectNamedPipe(pipeHandle)) + throw new Win32Exception(); + } +} diff --git a/src/NativeMethods.txt b/src/NativeMethods.txt new file mode 100644 index 00000000..0b3ca252 --- /dev/null +++ b/src/NativeMethods.txt @@ -0,0 +1,2 @@ +// Windows.Win32.System.Pipes +DisconnectNamedPipe diff --git a/src/packages.lock.json b/src/packages.lock.json index 7204d4ba..d3324a35 100644 --- a/src/packages.lock.json +++ b/src/packages.lock.json @@ -102,6 +102,17 @@ "resolved": "1.0.2739.15", "contentHash": "hZg7vHWBg4RIiK3rhkNqX3hcBqqoVXVrrO2BVrVOiSmEgfLa7lY64G1tWh2VY84MfHIFLCAQNwn3Rwvu+ICHyw==" }, + "Microsoft.Windows.CsWin32": { + "type": "Direct", + "requested": "[0.3.363, )", + "resolved": "0.3.363", + "contentHash": "Fd1p6Fr5H6A+c4FO033CojR8jKsvYZm6vigrKvB/auI40KXb0Vb+ge0F6baGBZptc1yIKVxmh/Iu8/KvLVQ94A==", + "dependencies": { + "Microsoft.Windows.SDK.Win32Docs": "0.1.42-alpha", + "Microsoft.Windows.SDK.Win32Metadata": "71.0.30-preview", + "Microsoft.Windows.WDK.Win32Metadata": "0.13.25-experimental" + } + }, "Nerdbank.GitVersioning": { "type": "Direct", "requested": "[3.10.85, )", @@ -217,6 +228,24 @@ "resolved": "1.6.22", "contentHash": "aBvunmrdu/x+4CaA/UP1Jx4xWGwk4kymhoIRnn2Vp+zi5/KOPQJ9EkSXHRUr01WcGKtYl3Au7XfkPJbU1G2sjQ==" }, + "Microsoft.Windows.SDK.Win32Docs": { + "type": "Transitive", + "resolved": "0.1.42-alpha", + "contentHash": "Z/9po23gUA9aoukirh2ItMU2ZS9++Js9Gdds9fu5yuMojDrmArvY2y+tq9985tR3cxFxpZO1O35Wjfo0khj5HA==" + }, + "Microsoft.Windows.SDK.Win32Metadata": { + "type": "Transitive", + "resolved": "71.0.30-preview", + "contentHash": "h/FD+SrIlxgr+HD/WrJnLvpTdyWFAX7F0FCMPRxPJFTt4Qz70wGXKEBZvvIDX7om4hP0Tb7O0s1ND3aUvigL8g==" + }, + "Microsoft.Windows.WDK.Win32Metadata": { + "type": "Transitive", + "resolved": "0.13.25-experimental", + "contentHash": "IM50tb/+UIwBr9FMr6ZKcZjCMW+Axo6NjGqKxgjUfyCY8dRnYUfrJEXxAaXoWtYP4X8EmASmC1Jtwh4XucseZg==", + "dependencies": { + "Microsoft.Windows.SDK.Win32Metadata": "63.0.31-preview" + } + }, "Newtonsoft.Json": { "type": "Transitive", "resolved": "13.0.3", diff --git a/test/Bravo.Tests/Infrastructure/SingleInstance/SingleInstanceServerTests.cs b/test/Bravo.Tests/Infrastructure/SingleInstance/SingleInstanceServerTests.cs index 0c85762c..b1f5389a 100644 --- a/test/Bravo.Tests/Infrastructure/SingleInstance/SingleInstanceServerTests.cs +++ b/test/Bravo.Tests/Infrastructure/SingleInstance/SingleInstanceServerTests.cs @@ -1,4 +1,5 @@ using System; +using System.IO; using System.IO.Pipes; using System.Text; using System.Threading; @@ -159,14 +160,20 @@ public void Activated_ClientConnectsButNeverWrites_RecoversAndKeepsListening() using (server) { using var errorRaised = new ManualResetEventSlim(); - server.Error += (_, _) => errorRaised.Set(); + Exception? error = null; + server.Error += (_, e) => + { + error = e.GetException(); + errorRaised.Set(); + }; using (var stalledClient = new NamedPipeClientStream( ".", options.PipeName, PipeDirection.Out, PipeOptions.CurrentUserOnly)) { stalledClient.Connect((int)s_timeout.TotalMilliseconds); - // Connected, but deliberately never writes: stands in for a stuck or hostile client. + // Connected, but never writes: stands in for a stuck or hostile client. Assert.True(errorRaised.Wait(s_timeout)); + Assert.IsType(error); } // The stalled connection released the pipe: a real client is served next. @@ -220,18 +227,26 @@ public void Activated_PayloadExceedsTheLimit_IsRejectedAndTheOwnerKeepsListening using var received = new ManualResetEventSlim(); using var rejected = new ManualResetEventSlim(); byte[]? payload = null; + Exception? error = null; server.Activated += (_, e) => { payload = e.Payload; received.Set(); }; - // An oversized payload is a reported rejection, not a silent drop: without this the owner - // would discard it with nothing recorded anywhere. - server.Error += (_, _) => rejected.Set(); + // An oversized payload is a reported rejection, not a silent drop. + server.Error += (_, e) => + { + error = e.GetException(); + rejected.Set(); + }; - Assert.True(SingleInstanceClient.Send(clientOptions, Payload(new string('x', 256))).IsDelivered); + // The owner stops reading past the limit and disconnects, so the client's write of the + // remaining bytes fails: a client with a larger limit learns that its payload was refused. + var result = SingleInstanceClient.Send(clientOptions, Payload(new string('x', 256))); + Assert.Equal(SingleInstanceSendStatus.Failed, result.Status); Assert.True(rejected.Wait(s_timeout)); + Assert.IsType(error); Assert.False(received.Wait(TimeSpan.FromSeconds(1))); Assert.True(SingleInstanceClient.Send(clientOptions, Payload("small")).IsDelivered); @@ -248,6 +263,53 @@ public void Send_PayloadExceedsTheConfiguredLimit_Throws() Assert.Throws(() => SingleInstanceClient.Send(options, Payload("far too long"))); } + [Fact] + public void Send_EmptyPayload_Throws() + { + var options = CreateOptions(); + + Assert.Throws(() => SingleInstanceClient.Send(options, Payload(""))); + } + + /// + /// A client that connects and closes without writing is reported, like an oversized payload, + /// and the owner keeps listening. + /// + [Fact] + public void Activated_ClientClosesWithoutWriting_IsReportedAndTheOwnerKeepsListening() + { + var options = CreateOptions(); + Assert.True(SingleInstanceServer.TryStart(options, out var server)); + + using (server) + { + using var errorRaised = new ManualResetEventSlim(); + Exception? error = null; + server.Error += (_, e) => + { + error = e.GetException(); + errorRaised.Set(); + }; + + using (var silentClient = new NamedPipeClientStream( + ".", options.PipeName, PipeDirection.Out, PipeOptions.CurrentUserOnly)) + { + silentClient.Connect((int)s_timeout.TotalMilliseconds); + } + + Assert.True(errorRaised.Wait(s_timeout)); + // Empty payload when the owner accepted before the close; otherwise the accept itself + // fails with ERROR_NO_DATA. + Assert.True(error is InvalidDataException or IOException, error?.ToString()); + + using var received = new ManualResetEventSlim(); + server.Activated += (_, _) => received.Set(); + + Assert.True(SingleInstanceClient.Send(options, Payload("hello")).IsDelivered); + Assert.True(received.Wait(s_timeout)); + } + } + [Fact] public void Dispose_CalledTwice_DoesNotThrow() {