Improve server shutdown logic (#2247)

Ensure connections abort when they don't close gracefully.

src/Kestrel.Core/Internal/HttpConnection.cs

@@ -21,7 +21,7 @@ using Microsoft.Extensions.Logging;
 
 namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
 {
-    public class HttpConnection : ITimeoutControl, IConnectionTimeoutFeature, IRequestProcessor
+    public class HttpConnection : ITimeoutControl, IConnectionTimeoutFeature
     {
         private static readonly ReadOnlyMemory<byte> Http2Id = new[] { (byte)'h', (byte)'2' };
 
@@ -32,6 +32,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
         private IPipeConnection _adaptedTransport;
 
         private readonly object _protocolSelectionLock = new object();
+        private ProtocolSelectionState _protocolSelectionState = ProtocolSelectionState.Initializing;
         private IRequestProcessor _requestProcessor;
         private Http1Connection _http1Connection;
 
@@ -54,7 +55,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
         public HttpConnection(HttpConnectionContext context)
         {
             _context = context;
-            _requestProcessor = this;
         }
 
         // For testing
@@ -140,19 +140,21 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
                 lock (_protocolSelectionLock)
                 {
                     // Ensure that the connection hasn't already been stopped.
-                    if (_requestProcessor == this)
+                    if (_protocolSelectionState == ProtocolSelectionState.Initializing)
                     {
                         switch (SelectProtocol())
                         {
                             case HttpProtocols.Http1:
                                 // _http1Connection must be initialized before adding the connection to the connection manager
                                 requestProcessor = _http1Connection = CreateHttp1Connection(_adaptedTransport, application);
+                                _protocolSelectionState = ProtocolSelectionState.Selected;
                                 break;
                             case HttpProtocols.Http2:
                                 // _http2Connection must be initialized before yielding control to the transport thread,
                                 // to prevent a race condition where _http2Connection.Abort() is called just as
                                 // _http2Connection is about to be initialized.
                                 requestProcessor = CreateHttp2Connection(_adaptedTransport, application);
+                                _protocolSelectionState = ProtocolSelectionState.Selected;
                                 break;
                             case HttpProtocols.None:
                                 // An error was already logged in SelectProtocol(), but we should close the connection.
@@ -197,6 +199,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
         internal void Initialize(IPipeConnection transport, IPipeConnection application)
         {
             _requestProcessor = _http1Connection = CreateHttp1Connection(transport, application);
+            _protocolSelectionState = ProtocolSelectionState.Selected;
         }
 
         private Http1Connection CreateHttp1Connection(IPipeConnection transport, IPipeConnection application)
@@ -241,8 +244,20 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
         {
             lock (_protocolSelectionLock)
             {
-                _requestProcessor?.StopProcessingNextRequest();
+                switch (_protocolSelectionState)
-                _requestProcessor = null;
+                {
+                    case ProtocolSelectionState.Initializing:
+                        CloseUninitializedConnection();
+                        _protocolSelectionState = ProtocolSelectionState.Stopped;
+                        break;
+                    case ProtocolSelectionState.Selected:
+                        _requestProcessor.StopProcessingNextRequest();
+                        _protocolSelectionState = ProtocolSelectionState.Stopping;
+                        break;
+                    case ProtocolSelectionState.Stopping:
+                    case ProtocolSelectionState.Stopped:
+                        break;
+                }
             }
 
             return _lifetimeTask;
@@ -252,8 +267,20 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
         {
             lock (_protocolSelectionLock)
             {
-                _requestProcessor?.Abort(ex);
+                switch (_protocolSelectionState)
-                _requestProcessor = null;
+                {
+                    case ProtocolSelectionState.Initializing:
+                        CloseUninitializedConnection();
+                        break;
+                    case ProtocolSelectionState.Selected:
+                    case ProtocolSelectionState.Stopping:
+                        _requestProcessor.Abort(ex);
+                        break;
+                    case ProtocolSelectionState.Stopped:
+                        break;
+                }
+
+                _protocolSelectionState = ProtocolSelectionState.Stopped;
             }
         }
 
@@ -261,7 +288,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
         {
             Abort(ex);
 
-            return _lifetimeTask;
+            return _socketClosedTcs.Task;
         }
 
         private async Task<Stream> ApplyConnectionAdaptersAsync()
@@ -582,20 +609,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
             _adaptedTransport.Output.Complete();
         }
 
-        // These IStoppableConnection methods only get called if the server shuts down during initialization.
+        private enum ProtocolSelectionState
-        Task IRequestProcessor.ProcessRequestsAsync<TContext>(IHttpApplication<TContext> application)
         {
-            throw new NotSupportedException();
+            Initializing,
-        }
+            Selected,
-
+            Stopping,
-        void IRequestProcessor.StopProcessingNextRequest()
+            Stopped
-        {
-            CloseUninitializedConnection();
-        }
-
-        void IRequestProcessor.Abort(Exception ex)
-        {
-            CloseUninitializedConnection();
         }
     }
 }

test/shared/TestApplicationErrorLogger.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Concurrent;
 using System.Linq;
+using Microsoft.AspNetCore.Server.Kestrel.Core.Internal;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
 using Microsoft.Extensions.Logging;
 
@@ -44,14 +45,23 @@ namespace Microsoft.AspNetCore.Testing
             if (logLevel == LogLevel.Critical && ThrowOnCriticalErrors)
 #endif
             {
-                Console.WriteLine($"Log {logLevel}[{eventId}]: {formatter(state, exception)} {exception?.Message}");
+                var log = $"Log {logLevel}[{eventId}]: {formatter(state, exception)} {exception?.Message}";
+
+                Console.WriteLine(log);
 
                 if (logLevel == LogLevel.Critical && ThrowOnCriticalErrors)
                 {
-                    throw new Exception("Unexpected critical error.", exception);
+                    throw new Exception($"Unexpected critical error. {log}", exception);
                 }
             }
 
+            // Fail tests where not all the connections close during server shutdown.
+            if (eventId.Id == 21 && eventId.Name == nameof(KestrelTrace.NotAllConnectionsAborted))
+            {
+                var log = $"Log {logLevel}[{eventId}]: {formatter(state, exception)} {exception?.Message}";
+                throw new Exception($"Shutdown failure. {log}");
+            }
+
             Messages.Enqueue(new LogMessage
             {
                 LogLevel = logLevel,