From 9b2b351fa678a729a1a5209a4539480d068ddf7b Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 21 Aug 2026 20:48:16 +1000 Subject: [PATCH] Bound the async exchange with the inline queue owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TrySendAsync had no deadline of any kind. Configure sets SendTimeout and ReceiveTimeout, which apply only to synchronous calls, and every async read and write used the caller's token — which is default from DiffRunner.AddInlineAsync, because Verify passes none, and likewise from AddDeleteAsync and InnerLaunchAsync. So an owner that accepted the connection and then stopped answering hung the failing test indefinitely. That is not hypothetical: the owner answers on its listener thread, so a connection can sit behind an accept that is itself waiting up to ten seconds on InlineApplier's cross process mutex, and a viewer stopped in a debugger does the same thing for as long as it is stopped. The synchronous TrySend gives up after three seconds; the async path, the one Verify actually takes, waited forever. Link a CancellationTokenSource with a 30 second deadline — longer than the sync wait to leave room for the applier mutex — and use its token everywhere the caller's was used. The token also closes the socket, because that is the only thing that unblocks every target: pre-net7 ReadToEndAsync takes no token at all and net462 has no cancellable connect or write either. A timeout is reported as absence, so the caller launches a viewer or stages the patch rather than waiting on a process that has stopped listening, and traced as "present but unresponsive" so the two stay tellable apart. port and wait overrides mirror the synchronous overload so the new test can hold a connection open on its own ephemeral port without touching anything static. --- src/DiffEngine.Tests/GlobalUsings.cs | 2 + src/DiffEngine.Tests/ViewerProtocolTests.cs | 42 +++++++++++ src/DiffEngine/Protocol/ViewerClient.cs | 81 ++++++++++++++++++--- 3 files changed, 113 insertions(+), 12 deletions(-) diff --git a/src/DiffEngine.Tests/GlobalUsings.cs b/src/DiffEngine.Tests/GlobalUsings.cs index 49fe999c..f54c8d6d 100644 --- a/src/DiffEngine.Tests/GlobalUsings.cs +++ b/src/DiffEngine.Tests/GlobalUsings.cs @@ -1,6 +1,8 @@ global using EmptyFiles; global using System.Collections.Concurrent; global using System.Diagnostics; +global using System.Net; +global using System.Net.Sockets; global using System.Reflection; global using System.Text; global using Polyfills; diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index eaa97cbb..56c058ae 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -673,6 +673,48 @@ public async Task ASecondBindIsRefused() await Assert.That(second).IsNull(); } + /// + /// An owner that accepts the connection and then says nothing. There used to be no bound on + /// this at all: SendTimeout and ReceiveTimeout apply only to synchronous calls, and the token + /// the async path was handed is the caller's, which is default from DiffRunner.AddInlineAsync. + /// A failing test waited for the owner for the rest of its life. + /// + [Test] + public async Task AnUnresponsiveOwnerTimesOutRatherThanHanging() + { + // Stop rather than Dispose: TcpListener is only IDisposable on the modern frameworks, and + // this test compiles for net48 too + var listener = new TcpListener(IPAddress.Loopback, 0); + listener.Start(); + try + { + var port = ((IPEndPoint) listener.LocalEndpoint).Port; + + // Accepted and then held, which is what a viewer inside the applier mutex looks like. + // Kept in scope so the connection is not collected and closed under the client + var accepted = listener.AcceptTcpClientAsync(); + + var watch = Stopwatch.StartNew(); + var sent = await ViewerClient.TrySendAsync( + new(ViewerVerb.List), + default, + port, + TimeSpan.FromSeconds(1)); + watch.Stop(); + + await Assert.That(sent).IsFalse(); + await Assert.That(watch.Elapsed).IsLessThan(TimeSpan.FromSeconds(15)); + + if (accepted.Status == TaskStatus.RanToCompletion) + { + accepted.Result.Close(); + } + } + finally + { + listener.Stop(); + } + } [Test] public async Task AnAbsentOwnerIsNotAnError() { diff --git a/src/DiffEngine/Protocol/ViewerClient.cs b/src/DiffEngine/Protocol/ViewerClient.cs index 01a94baf..9f36941a 100644 --- a/src/DiffEngine/Protocol/ViewerClient.cs +++ b/src/DiffEngine/Protocol/ViewerClient.cs @@ -36,6 +36,17 @@ public static int Port static readonly TimeSpan timeout = TimeSpan.FromSeconds(3); + /// + /// The deadline for the async exchange. Longer than the synchronous one because the owner + /// answers on its listener thread, so a connection can sit behind an accept that is itself + /// waiting up to ten seconds on 's cross process mutex. Shorter + /// than forever because there was no bound at all: SendTimeout and ReceiveTimeout apply only + /// to synchronous calls, and the token every async call was given is the caller's, which is + /// default from DiffRunner.AddInlineAsync - Verify passes none. An owner that accepted the + /// connection and then stopped answering hung the failing test for good. + /// + static readonly TimeSpan asyncTimeout = TimeSpan.FromSeconds(30); + /// /// For callers on a clock or an interactive path, such as the tray's scan timer and its menu. /// The exchange is loopback to a local process, so anything slower than this is a wedged owner @@ -95,40 +106,71 @@ public static bool TrySend( /// Fully async, including the read. A blocking read here would tie up a thread pool thread for /// the whole exchange, and a parallel test run calling this once per failing snapshot would /// starve the pool on a small machine. + /// + /// and override and + /// for a single call, as they do on the synchronous overload. Tests + /// pass their own ephemeral port rather than mutating anything static, so they can run in + /// parallel. + /// /// - public static async Task TrySendAsync(ViewerMessage message, Cancel cancel) + public static async Task TrySendAsync( + ViewerMessage message, + Cancel cancel, + int? port = null, + TimeSpan? wait = null) { + var endpointPort = port ?? Port; + var timeToWait = wait ?? asyncTimeout; + using var deadline = CancelSource.CreateLinkedTokenSource(cancel); + deadline.CancelAfter(timeToWait); + var token = deadline.Token; try { using var client = new TcpClient(); + // Closing the socket is the only thing that unblocks every framework: the pre-net7 + // ReadToEndAsync takes no token at all, and net462 has no cancellable connect or + // write either. Registered after the client and so disposed before it, which is what + // stops the callback firing on a disposed object + using var abort = token.Register(() => Abort(client)); #if NET6_0_OR_GREATER - await client.ConnectAsync(IPAddress.Loopback, Port, cancel); + await client.ConnectAsync(IPAddress.Loopback, endpointPort, token); #else - cancel.ThrowIfCancellationRequested(); - using (cancel.Register(client.Close)) - { - await client.ConnectAsync(IPAddress.Loopback, Port); - } + token.ThrowIfCancellationRequested(); + await client.ConnectAsync(IPAddress.Loopback, endpointPort); #endif - Configure(client, timeout); + Configure(client, timeToWait); var stream = client.GetStream(); var bytes = Encoding.UTF8.GetBytes(message.Build()); #if NET6_0_OR_GREATER - await stream.WriteAsync(bytes, cancel); + await stream.WriteAsync(bytes, token); #else - await stream.WriteAsync(bytes, 0, bytes.Length, cancel); + await stream.WriteAsync(bytes, 0, bytes.Length, token); #endif - await stream.FlushAsync(cancel); + await stream.FlushAsync(token); HalfClose(client); using var reader = new StreamReader(stream, Encoding.UTF8); #if NET7_0_OR_GREATER - var text = await reader.ReadToEndAsync(cancel); + var text = await reader.ReadToEndAsync(token); #else var text = await reader.ReadToEndAsync(); #endif return ViewerResponse.TryParse(text, out var response) && response.Ok; } + // The deadline, rather than the caller cancelling. Whatever the abort surfaced as - a + // cancellation, a closed socket, a torn down stream - the owner is present but not + // answering. Reported as absence because that is the recoverable answer: the caller + // launches a viewer or stages the patch, rather than waiting on a process that has + // stopped listening. Logged so the two are still tellable apart afterwards + catch (Exception exception) + when (!cancel.IsCancellationRequested && token.IsCancellationRequested) + { + // Trace rather than Logging, because this file is linked into the viewer too + Trace.WriteLine( + $"Timed out after {timeToWait} waiting for the inline queue owner on port {endpointPort}. " + + $"Verb: {message.Verb}. The owner is present but unresponsive. {exception.GetType().Name}"); + return false; + } // Cancellation is the caller's business; a missing owner is not. catch (Exception exception) when (exception is not OperationCanceledException && Ignorable(exception)) @@ -137,6 +179,21 @@ public static async Task TrySendAsync(ViewerMessage message, Cancel cancel } } + /// + /// Unblocks whatever the exchange is waiting on. Swallowing here rather than letting it out: + /// this runs on the timer that fired the deadline, where a throw has nowhere to go. + /// + static void Abort(TcpClient client) + { + try + { + client.Close(); + } + catch (Exception exception) + when (Ignorable(exception)) + { + } + } static void Configure(TcpClient client, TimeSpan wait) { client.SendTimeout = (int) wait.TotalMilliseconds;