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;