From d45f31e5e437992fc093ba006e92fd6e02ee3b7d Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 10:15:33 +1000 Subject: [PATCH] Do not give up the queue because one accept failed Listen returned on any SocketException from Accept, without looking at the error code or at cancellation. That treats a failure of one accept as a failure of the listener, and the two are not the same thing at all. A peer that resets while its connection is still sitting in the backlog surfaces exactly this way - WSAECONNRESET on Windows, ECONNABORTED on BSD and macOS - and is common enough that Kestrel retries it by name. The consequence here is worse than a dropped connection: the socket stays bound, so nobody else can take the queue for the life of the process, and every later client lands in a backlog that nothing is draining. PiperServer already continues in the same situation. Return only when cancelled, or on OperationAborted and Interrupted, which are how a stopped listener reports itself when the token has not been observed yet. The tests pin the rule rather than the race. I wrote an end to end one first - twenty abortive closes, then a real exchange - and deleted it after confirming it passes with the old `return` still in place: on Windows the accept succeeds and the reset surfaces later, during the read, which a different catch already handles. A test that cannot fail is worse than no test. --- src/DiffEngine.Tests/ViewerProtocolTests.cs | 36 +++++++++++++++++++++ src/DiffEngine/Protocol/ViewerServer.cs | 21 +++++++++++- 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index 7ed60f8e..2b7b5423 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -733,6 +733,42 @@ public async Task AnUnresponsiveOwnerTimesOutRatherThanHanging() listener.Stop(); } } + /// + /// Which socket failures mean the listener has stopped, as against one accept having failed. + /// + /// Returning on any SocketException gave the queue away for the life of the process: the + /// socket stays bound so nobody else can take it, and every later client lands in a backlog + /// nothing is draining. A peer that resets while its connection sits in that backlog is the + /// ordinary way to hit it - WSAECONNRESET on Windows, ECONNABORTED on BSD and macOS - and is + /// why Kestrel retries the same condition. + /// + /// + [Test] + [Arguments(SocketError.OperationAborted, true)] + [Arguments(SocketError.Interrupted, true)] + [Arguments(SocketError.ConnectionReset, false)] + [Arguments(SocketError.ConnectionAborted, false)] + [Arguments(SocketError.NetworkDown, false)] + public async Task SocketFailuresThatStopTheListener(SocketError error, bool expected) + { + var exception = new SocketException((int) error); + + await Assert.That(ViewerServer.IsStop(exception, default)).IsEqualTo(expected); + } + + /// + /// And a cancelled token means stop whatever the code says, since that is the ordinary way a + /// listener is shut down and the token may be observed before the exception is. + /// + [Test] + public async Task ACancelledTokenStopsTheListenerWhateverTheCode() + { + using var cancel = new CancelSource(); + await cancel.CancelAsync(); + + await Assert.That(ViewerServer.IsStop(new((int) SocketError.ConnectionReset), cancel.Token)).IsTrue(); + } + [Test] public async Task AnAbsentOwnerIsNotAnError() { diff --git a/src/DiffEngine/Protocol/ViewerServer.cs b/src/DiffEngine/Protocol/ViewerServer.cs index 93a6cfba..5c568708 100644 --- a/src/DiffEngine/Protocol/ViewerServer.cs +++ b/src/DiffEngine/Protocol/ViewerServer.cs @@ -76,10 +76,20 @@ public async Task Listen(Func handle, Cancel canc // Same, on the frameworks where a stopped listener reports it this way. return; } - catch (SocketException) + catch (SocketException exception) + when (IsStop(exception, cancel)) { return; } + catch (SocketException) + { + // A failure of one accept rather than of the listener. A peer that resets while + // its connection sits in the backlog surfaces exactly this way - WSAECONNRESET on + // Windows, ECONNABORTED on BSD and macOS - and returning gave the queue away for + // the life of the process: the socket stays bound, so nobody else can take it, + // and every later client lands in a backlog nothing is draining + continue; + } // Each connection on its own task, so one slow exchange does not stop the next from // being answered. Accepting an inline snapshot legitimately takes seconds, and a @@ -88,6 +98,15 @@ public async Task Listen(Func handle, Cancel canc } } + /// + /// Whether a socket failure means the listener itself has stopped, rather than one accept + /// having failed. Cancellation is the ordinary way that happens; the two error codes are how a + /// stopped listener reports itself when the token has not been observed yet. + /// + internal static bool IsStop(SocketException exception, Cancel cancel) => + cancel.IsCancellationRequested || + exception.SocketErrorCode is SocketError.OperationAborted or SocketError.Interrupted; + // ReSharper disable once ReplaceAsyncWithTaskReturn async Task Accept(Cancel cancel) {