Reduce first chance ODEs in SocketConnection.DoReceive() (#48176)

src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs

@@ -23,7 +23,6 @@ internal sealed partial class SocketConnection : TransportConnection
     private readonly CancellationTokenSource _connectionClosedTokenSource = new CancellationTokenSource();
 
     private readonly object _shutdownLock = new object();
-    private volatile bool _socketDisposed;
     private volatile Exception? _shutdownReason;
     private Task? _sendingTask;
     private Task? _receivingTask;
@@ -133,7 +132,7 @@ internal sealed partial class SocketConnection : TransportConnection
 
         try
         {
-            while (true)
+            while (_shutdownReason is null)
             {
                 if (_waitForData)
                 {
@@ -191,6 +190,14 @@ internal sealed partial class SocketConnection : TransportConnection
 
                 bool IsNormalCompletion(SocketOperationResult result)
                 {
+                    // There's still a small chance that both DoReceive() and DoSend() can log the same connection reset.
+                    // Both logs will have the same ConnectionId. I don't think it's worthwhile to lock just to avoid this.
+                    // When _shutdownReason is set, error is ignored, so it does not need to be initialized.
+                    if (_shutdownReason is not null)
+                    {
+                        return false;
+                    }
+
                     if (!result.HasError)
                     {
                         return true;
@@ -198,30 +205,20 @@ internal sealed partial class SocketConnection : TransportConnection
 
                     if (IsConnectionResetError(result.SocketError.SocketErrorCode))
                     {
-                        // This could be ignored if _shutdownReason is already set.
                         var ex = result.SocketError;
                         error = new ConnectionResetException(ex.Message, ex);
 
-                        // There's still a small chance that both DoReceive() and DoSend() can log the same connection reset.
-                        // Both logs will have the same ConnectionId. I don't think it's worthwhile to lock just to avoid this.
-                        if (!_socketDisposed)
-                        {
-                            SocketsLog.ConnectionReset(_logger, this);
-                        }
+                        SocketsLog.ConnectionReset(_logger, this);
 
                         return false;
                     }
 
                     if (IsConnectionAbortError(result.SocketError.SocketErrorCode))
                     {
-                        // This exception should always be ignored because _shutdownReason should be set.
                         error = result.SocketError;
 
-                        if (!_socketDisposed)
-                        {
-                            // This is unexpected if the socket hasn't been disposed yet.
-                            SocketsLog.ConnectionError(_logger, this, error);
-                        }
+                        // This is unexpected if the socket hasn't been disposed yet.
+                        SocketsLog.ConnectionError(_logger, this, error);
 
                         return false;
                     }
@@ -239,7 +236,7 @@ internal sealed partial class SocketConnection : TransportConnection
             // This exception should always be ignored because _shutdownReason should be set.
             error = ex;
 
-            if (!_socketDisposed)
+            if (_shutdownReason is not null)
             {
                 // This is unexpected if the socket hasn't been disposed yet.
                 SocketsLog.ConnectionError(_logger, this, error);
@@ -366,19 +363,18 @@ internal sealed partial class SocketConnection : TransportConnection
     {
         lock (_shutdownLock)
         {
-            if (_socketDisposed)
+            if (_shutdownReason is not null)
             {
                 return;
             }
 
-            // Make sure to close the connection only after the _aborted flag is set.
+            // Make sure to dispose the socket after the volatile _shutdownReason is set.
             // Without this, the RequestsCanBeAbortedMidRead test will sometimes fail when
             // a BadHttpRequestException is thrown instead of a TaskCanceledException.
-            _socketDisposed = true;
-
-            // shutdownReason should only be null if the output was completed gracefully, so no one should ever
-            // ever observe the nondescript ConnectionAbortedException except for connection middleware attempting
-            // to half close the connection which is currently unsupported.
+            //
+            // The shutdownReason argument should only be null if the output was completed gracefully, so no one should ever
+            // ever observe this ConnectionAbortedException except for connection middleware attempting
+            // to half close the connection which is currently unsupported. The message is always logged though.
             _shutdownReason = shutdownReason ?? new ConnectionAbortedException("The Socket transport's send loop completed gracefully.");
             SocketsLog.ConnectionWriteFin(_logger, this, _shutdownReason.Message);