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;