From 3dae2c8d546631a64a98c3a0926c34a5e52679b8 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Tue, 22 Sep 2026 08:47:18 +1000 Subject: [PATCH] Answer the viewer's port while its render thread is busy ViewerServer.Listen awaited its accept without ConfigureAwait(false), and the Windows viewer calls it from its UI thread once the form exists, when that thread already carries a WinForms synchronization context. So every accept resumed on the UI thread, which is the render loop and only pumps between frames. While the render thread was busy - an owning viewer applying an accept waits up to ten seconds on InlineApplier's cross process mutex - no new connection was handled at all: the tray's listings, an attached viewer's polls and the next failing snapshot all waited for the render loop to come round. Reproduced against the real viewer: with one source file's patch mutex held and Accept clicked on that entry, a listfull sent to the viewer did not answer within a second for the whole five seconds the mutex was held, and answered at once after it was let go. With this change it answers throughout. Both awaits in the accept chain take ConfigureAwait(false). The first Accept runs on the caller's thread, so fixing Listen's await alone still parks Accept's continuation on the UI thread. Each connection's handler already ran on the pool. The Mac and Linux heads install no context and were never affected. AnOwnerAnswersWhileTheThreadThatStartedItIsBusy starts the listener under a single threaded context that is never pumped, and asserts a client is answered. It times out on the old code, and on the new code with either await left as it was. --- claude.md | 4 ++ src/DiffEngine.Tests/ViewerProtocolTests.cs | 53 +++++++++++++++++++++ src/DiffEngine/Protocol/ViewerServer.cs | 11 +++-- 3 files changed, 65 insertions(+), 3 deletions(-) diff --git a/claude.md b/claude.md index e7659518..55140ef9 100644 --- a/claude.md +++ b/claude.md @@ -223,6 +223,10 @@ apart. quotes, braces and newlines, and the `inline` body carries an `InlinePatchFile` payload verbatim. - Compiles for every DiffEngine target, so the socket calls carry `#if` branches for the frameworks with no cancellation overloads. `ViewerProtocolTests` runs on all of them. +- `ViewerServer`'s accept loop awaits with `ConfigureAwait(false)`, the one place that matters + in a repo that otherwise leaves it off. The Windows viewer starts listening on its UI thread, + and resuming there left every connection waiting on the render loop to pump. + `AnOwnerAnswersWhileTheThreadThatStartedItIsBusy` pins it. **Native shim (`native/`), used by the Mac and Linux heads only:** - `raylib` and `imgui` are fetched by CMake (`FetchContent`), pinned by tag in diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index 7ebdbe6e..be99321e 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -639,6 +639,59 @@ public async Task AnOwnerAnswersAClient() await Wait(listening); } + /// + /// The Windows viewer starts listening on its UI thread, which carries a WinForms context by + /// then, and that thread is the render loop: it pumps between frames, and not at all while an + /// accept on it waits up to ten seconds on InlineApplier's mutex. The accept loop resumed on + /// it, so every connection went unanswered for as long as the render thread was busy - a + /// tray's listing, an attached viewer's poll, the next failing snapshot. + /// + /// A context that is never pumped is that thread at its worst, and the owner still answers. + /// + /// + [Test] + public async Task AnOwnerAnswersWhileTheThreadThatStartedItIsBusy() + { + await Assert.That(ViewerServer.TryBind(0, out var bound)).IsTrue(); + using var server = bound!; + using var cancel = new CancelSource(); + Task listening; + var previous = SynchronizationContext.Current; + // Only around the call, and with nothing awaited inside it: a continuation of this test + // posted to a context nobody pumps would never run + SynchronizationContext.SetSynchronizationContext(new UnpumpedContext()); + try + { + listening = server.Listen(_ => ViewerResponse.Success($"heard {_.Verb}"), cancel.Token); + } + finally + { + SynchronizationContext.SetSynchronizationContext(previous); + } + + var sent = ViewerClient.TrySend(new(ViewerVerb.List), out var response, server.Port, underLoad); + + await Assert.That(sent).IsTrue(); + await Assert.That(response!.Message).IsEqualTo("heard List"); + + await cancel.CancelAsync(); + await Wait(listening); + } + + /// + /// The one thread of a context whose owner is too busy to pump it, so whatever is posted to it + /// waits for good. + /// + sealed class UnpumpedContext : SynchronizationContext + { + public override void Post(SendOrPostCallback callback, object? state) + { + } + + public override void Send(SendOrPostCallback callback, object? state) => + throw new NotSupportedException("A thread that is not pumping cannot be sent to."); + } + /// /// Connections are handled concurrently, so one slow exchange does not stop the next from /// being answered. Accepting an inline snapshot legitimately takes seconds, and a client diff --git a/src/DiffEngine/Protocol/ViewerServer.cs b/src/DiffEngine/Protocol/ViewerServer.cs index 5c568708..d8bf3ca3 100644 --- a/src/DiffEngine/Protocol/ViewerServer.cs +++ b/src/DiffEngine/Protocol/ViewerServer.cs @@ -60,7 +60,12 @@ public async Task Listen(Func handle, Cancel canc TcpClient client; try { - client = await Accept(cancel); + // Off whatever thread started listening, here and in Accept. The Windows viewer + // starts on its UI thread, which has a WinForms context by then, and resuming there + // waited for the render loop to pump: every connection went unanswered for as long + // as that thread was busy, which an accept holding InlineApplier's mutex makes up + // to ten seconds. Both awaits, because the first Accept runs on the caller's thread. + client = await Accept(cancel).ConfigureAwait(false); } catch (OperationCanceledException) { @@ -111,12 +116,12 @@ internal static bool IsStop(SocketException exception, Cancel cancel) => async Task Accept(Cancel cancel) { #if NET6_0_OR_GREATER - return await listener.AcceptTcpClientAsync(cancel); + return await listener.AcceptTcpClientAsync(cancel).ConfigureAwait(false); #else // No token overload here, so cancellation arrives as the registered Stop, which faults // this await with one of the exceptions the caller already treats as "stop serving". cancel.ThrowIfCancellationRequested(); - return await listener.AcceptTcpClientAsync(); + return await listener.AcceptTcpClientAsync().ConfigureAwait(false); #endif }