From 374ba3603ed6be5fa6f3f680dc1a985d7ac96280 Mon Sep 17 00:00:00 2001 From: Alberto Spelta Date: Mon, 19 Oct 2026 21:15:58 +0200 Subject: [PATCH] refactor: harden the single-instance pipe server Port the single-instance changes: IInstanceActivationEvents becomes IInstanceEvents, the server reports failures through ErrorEventArgs, rejects empty payloads and bounds the read buffer, and the client reports a refused payload as a failed send. Fix a stuck owner: when a client closed during the accept, the failed WaitForConnectionAsync left the stream waiting to connect, where NamedPipeServerStream.Disconnect throws without disconnecting. The instance stayed closing and every later accept failed, so the owner kept the pipe name but answered nobody. The server now disconnects the instance through DisconnectNamedPipe, generated by CsWin32. --- src/Bravo.csproj | 1 + src/Host/BravoApplicationInstance.cs | 40 ++- src/Host/BravoServiceCollectionExtensions.cs | 2 +- ...ActivationEvents.cs => IInstanceEvents.cs} | 5 +- .../InstanceActivationRequestedEventArgs.cs | 2 +- src/Host/InstancePipeName.cs | 45 ++-- src/Infrastructure/AppWindow.cs | 14 +- .../SingleInstanceActivatedEventArgs.cs | 9 +- .../SingleInstance/SingleInstanceClient.cs | 10 +- .../SingleInstanceErrorEventArgs.cs | 13 - .../SingleInstance/SingleInstanceOptions.cs | 35 +-- .../SingleInstanceSendResult.cs | 26 +- .../SingleInstance/SingleInstanceServer.cs | 245 ++++++++++-------- .../Windows/Interop/PipeInterop.cs | 18 ++ src/NativeMethods.txt | 2 + src/packages.lock.json | 29 +++ .../SingleInstanceServerTests.cs | 74 +++++- 17 files changed, 345 insertions(+), 225 deletions(-) rename src/Host/{IInstanceActivationEvents.cs => IInstanceEvents.cs} (69%) delete mode 100644 src/Infrastructure/SingleInstance/SingleInstanceErrorEventArgs.cs create mode 100644 src/Infrastructure/Windows/Interop/PipeInterop.cs create mode 100644 src/NativeMethods.txt 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() {