Do not give up the queue because one accept failed (#790)

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.

Author: SimonCropp

src/DiffEngine.Tests/ViewerProtocolTests.cs

@@ -741,6 +741,42 @@ public async Task AnUnresponsiveOwnerTimesOutRatherThanHanging()
             listener.Stop();
         }
     }
+    /// <summary>
+    /// Which socket failures mean the listener has stopped, as against one accept having failed.
+    /// <para>
+    /// 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.
+    /// </para>
+    /// </summary>
+    [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);
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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();
+    }
+
     /// <summary>
     /// An owner that answers with an error is not an absent one. Collapsing the two into false
     /// meant a refused inline was read as "nobody is there", so a second viewer was launched, it

src/DiffEngine/Protocol/ViewerServer.cs

@@ -76,10 +76,20 @@ public async Task Listen(Func<ViewerMessage, ViewerResponse> 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<ViewerMessage, ViewerResponse> handle, Cancel canc
         }
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    internal static bool IsStop(SocketException exception, Cancel cancel) =>
+        cancel.IsCancellationRequested ||
+        exception.SocketErrorCode is SocketError.OperationAborted or SocketError.Interrupted;
+
     // ReSharper disable once ReplaceAsyncWithTaskReturn
     async Task<TcpClient> Accept(Cancel cancel)
     {