Merged PR 32090: [7.0] Forcibly close socket on abort

Forcibly close socket when connection is aborted.
dotnet · Jul 15, 2023 · 5a4c82e · 5a4c82e
1 parent 5b4c366
commit 5a4c82e
Show file tree

Hide file tree

Showing 15 changed files with 443 additions and 64 deletions.
diff --git a/src/Servers/IIS/IIS/test/Common.LongTests/ShutdownTests.cs b/src/Servers/IIS/IIS/test/Common.LongTests/ShutdownTests.cs
@@ -535,8 +535,15 @@ public async Task ClosesConnectionOnServerAbortOutOfProcess()
             var response = await deploymentResult.HttpClient.GetAsync("/Abort").TimeoutAfter(TimeoutExtensions.DefaultTimeoutValue);
 
             Assert.Equal(HttpStatusCode.BadGateway, response.StatusCode);
+
+#if NEWSHIM_FUNCTIONALS
+            // In-proc SocketConnection isn't used and there's no abort
             // 0x80072f78 ERROR_HTTP_INVALID_SERVER_RESPONSE The server returned an invalid or unrecognized response
             Assert.Contains("0x80072f78", await response.Content.ReadAsStringAsync());
+#else
+            // 0x80072efe ERROR_INTERNET_CONNECTION_ABORTED The connection with the server was terminated abnormally
+            Assert.Contains("0x80072efe", await response.Content.ReadAsStringAsync());
+#endif
         }
         catch (HttpRequestException)
         {

diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs
@@ -98,7 +98,14 @@ protected override void OnRequestProcessingEnded()
         _http1Output.Dispose();
     }
 
-    public void OnInputOrOutputCompleted()
+    void IRequestProcessor.OnInputOrOutputCompleted()
+    {
+        // Closed gracefully.
+        _http1Output.Abort(ServerOptions.FinOnError ? new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient) : null!);
+        CancelRequestAbortedToken();
+    }
+
+    void IHttpOutputAborter.OnInputOrOutputCompleted()
     {
         _http1Output.Abort(new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient));
         CancelRequestAbortedToken();

diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs
@@ -204,7 +204,7 @@ protected void ThrowUnexpectedEndOfRequestContent()
         // so we call OnInputOrOutputCompleted() now to prevent a race in our tests where a 400
         // response is written after observing the unexpected end of request content instead of just
         // closing the connection without a response as expected.
-        _context.OnInputOrOutputCompleted();
+        ((IHttpOutputAborter)_context).OnInputOrOutputCompleted();
 
         KestrelBadHttpRequestException.Throw(RequestRejectionReason.UnexpectedEndOfRequestContent);
     }

diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs
@@ -159,7 +159,8 @@ public Http2Connection(HttpConnectionContext context)
     public void OnInputOrOutputCompleted()
     {
         TryClose();
-        _frameWriter.Abort(new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient));
+        var useException = _context.ServiceContext.ServerOptions.FinOnError || _clientActiveStreamCount != 0;
+        _frameWriter.Abort(useException ? new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient) : null!);
     }
 
     public void Abort(ConnectionAbortedException ex)

diff --git a/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs b/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs
@@ -26,10 +26,20 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core;
 public class KestrelServerOptions
 {
     internal const string DisableHttp1LineFeedTerminatorsSwitchKey = "Microsoft.AspNetCore.Server.Kestrel.DisableHttp1LineFeedTerminators";
+    private const string FinOnErrorSwitch = "Microsoft.AspNetCore.Server.Kestrel.FinOnError";
+    private static readonly bool _finOnError;
+
+    static KestrelServerOptions()
+    {
+        AppContext.TryGetSwitch(FinOnErrorSwitch, out _finOnError);
+    }
 
     // internal to fast-path header decoding when RequestHeaderEncodingSelector is unchanged.
     internal static readonly Func<string, Encoding?> DefaultHeaderEncodingSelector = _ => null;
 
+    // Opt-out flag for back compat. Remove in 9.0 (or make public).
+    internal bool FinOnError { get; set; } = _finOnError;
+
     private Func<string, Encoding?> _requestHeaderEncodingSelector = DefaultHeaderEncodingSelector;
 
     private Func<string, Encoding?> _responseHeaderEncodingSelector = DefaultHeaderEncodingSelector;

diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs
@@ -30,6 +30,7 @@ internal sealed partial class SocketConnection : TransportConnection
     private readonly TaskCompletionSource _waitForConnectionClosedTcs = new TaskCompletionSource();
     private bool _connectionClosed;
     private readonly bool _waitForData;
+    private readonly bool _finOnError;
 
     internal SocketConnection(Socket socket,
                               MemoryPool<byte> memoryPool,
@@ -38,7 +39,8 @@ internal SocketConnection(Socket socket,
                               SocketSenderPool socketSenderPool,
                               PipeOptions inputOptions,
                               PipeOptions outputOptions,
-                              bool waitForData = true)
+                              bool waitForData = true,
+                              bool finOnError = false)
     {
         Debug.Assert(socket != null);
         Debug.Assert(memoryPool != null);
@@ -49,6 +51,7 @@ internal SocketConnection(Socket socket,
         _logger = logger;
         _waitForData = waitForData;
         _socketSenderPool = socketSenderPool;
+        _finOnError = finOnError;
 
         LocalEndPoint = _socket.LocalEndPoint;
         RemoteEndPoint = _socket.RemoteEndPoint;
@@ -380,11 +383,21 @@ private void Shutdown(Exception? shutdownReason)
             // ever observe the nondescript ConnectionAbortedException except for connection middleware attempting
             // to half close the connection which is currently unsupported.
             _shutdownReason = shutdownReason ?? new ConnectionAbortedException("The Socket transport's send loop completed gracefully.");
+
+            // NB: not _shutdownReason since we don't want to do this on graceful completion
+            if (!_finOnError && shutdownReason is not null)
+            {
+                SocketsLog.ConnectionWriteRst(_logger, this, shutdownReason.Message);
+
+                // This forces an abortive close with linger time 0 (and implies Dispose)
+                _socket.Close(timeout: 0);
+                return;
+            }
+
             SocketsLog.ConnectionWriteFin(_logger, this, _shutdownReason.Message);
 
             try
             {
-                // Try to gracefully close the socket even for aborts to match libuv behavior.
                 _socket.Shutdown(SocketShutdown.Both);
             }
             catch

diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketsLog.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketsLog.cs
@@ -31,6 +31,17 @@ public static void ConnectionWriteFin(ILogger logger, SocketConnection connectio
         }
     }
 
+    [LoggerMessage(8, LogLevel.Debug, @"Connection id ""{ConnectionId}"" sending RST because: ""{Reason}""", EventName = "ConnectionWriteRst", SkipEnabledCheck = true)]
+    private static partial void ConnectionWriteRstCore(ILogger logger, string connectionId, string reason);
+
+    public static void ConnectionWriteRst(ILogger logger, SocketConnection connection, string reason)
+    {
+        if (logger.IsEnabled(LogLevel.Debug))
+        {
+            ConnectionWriteRstCore(logger, connection.ConnectionId, reason);
+        }
+    }
+
     // Reserved: Event ID 11, EventName = ConnectionWrite
 
     // Reserved: Event ID 12, EventName = ConnectionWriteCallback

diff --git a/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionContextFactory.cs b/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionContextFactory.cs
@@ -113,7 +113,8 @@ public ConnectionContext Create(Socket socket)
             setting.SocketSenderPool,
             setting.InputOptions,
             setting.OutputOptions,
-            waitForData: _options.WaitForDataBeforeAllocatingBuffer);
+            waitForData: _options.WaitForDataBeforeAllocatingBuffer,
+            finOnError: _options.FinOnError);
 
         connection.Start();
         return connection;

diff --git a/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionFactoryOptions.cs b/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionFactoryOptions.cs
@@ -23,8 +23,12 @@ internal SocketConnectionFactoryOptions(SocketTransportOptions transportOptions)
         MaxWriteBufferSize = transportOptions.MaxWriteBufferSize;
         UnsafePreferInlineScheduling = transportOptions.UnsafePreferInlineScheduling;
         MemoryPoolFactory = transportOptions.MemoryPoolFactory;
+        FinOnError = transportOptions.FinOnError;
     }
 
+    // Opt-out flag for back compat. Remove in 9.0 (or make public).
+    internal bool FinOnError { get; set; }
+
     /// <summary>
     /// The number of I/O queues used to process requests. Set to 0 to directly schedule I/O to the ThreadPool.
     /// </summary>

diff --git a/src/Servers/Kestrel/Transport.Sockets/src/SocketTransportOptions.cs b/src/Servers/Kestrel/Transport.Sockets/src/SocketTransportOptions.cs
@@ -13,6 +13,17 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets;
 /// </summary>
 public class SocketTransportOptions
 {
+    private const string FinOnErrorSwitch = "Microsoft.AspNetCore.Server.Kestrel.FinOnError";
+    private static readonly bool _finOnError;
+
+    static SocketTransportOptions()
+    {
+        AppContext.TryGetSwitch(FinOnErrorSwitch, out _finOnError);
+    }
+
+    // Opt-out flag for back compat. Remove in 9.0 (or make public).
+    internal bool FinOnError { get; set; } = _finOnError;
+
     /// <summary>
     /// The number of I/O queues used to process requests. Set to 0 to directly schedule I/O to the ThreadPool.
     /// </summary>

diff --git a/src/Servers/Kestrel/shared/test/StreamBackedTestConnection.cs b/src/Servers/Kestrel/shared/test/StreamBackedTestConnection.cs
@@ -138,7 +138,7 @@ public async Task ReceiveEnd(params string[] lines)
     public async Task WaitForConnectionClose()
     {
         var buffer = new byte[128];
-        var bytesTransferred = await _stream.ReadAsync(buffer, 0, 128).TimeoutAfter(Timeout);
+        var bytesTransferred = await _stream.ReadAsync(buffer, 0, 128).ContinueWith(t => t.IsFaulted ? 0 : t.Result).TimeoutAfter(Timeout);
 
         if (bytesTransferred > 0)
         {

diff --git a/src/Servers/Kestrel/shared/test/TransportTestHelpers/TestServer.cs b/src/Servers/Kestrel/shared/test/TransportTestHelpers/TestServer.cs
@@ -48,7 +48,7 @@ public TestServer(RequestDelegate app, TestServiceContext context, ListenOptions
     {
     }
 
-    public TestServer(RequestDelegate app, TestServiceContext context, Action<ListenOptions> configureListenOptions)
+    public TestServer(RequestDelegate app, TestServiceContext context, Action<ListenOptions> configureListenOptions, Action<IServiceCollection> configureServices = null)
         : this(app, context, options =>
         {
             var listenOptions = new ListenOptions(new IPEndPoint(IPAddress.Loopback, 0))
@@ -57,7 +57,10 @@ public TestServer(RequestDelegate app, TestServiceContext context, Action<Listen
             };
             configureListenOptions(listenOptions);
             options.CodeBackedListenOptions.Add(listenOptions);
-        }, _ => { })
+        }, s =>
+        {
+            configureServices?.Invoke(s);
+        })
     {
     }
 

diff --git a/src/Servers/Kestrel/test/FunctionalTests/Http2/ShutdownTests.cs b/src/Servers/Kestrel/test/FunctionalTests/Http2/ShutdownTests.cs
@@ -44,6 +44,60 @@ public ShutdownTests()
         };
     }
 
+    [ConditionalFact]
+    public async Task ConnectionClosedWithoutActiveRequestsOrGoAwayFIN()
+    {
+        var connectionClosed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        var readFin = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        var writeFin = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+
+        TestSink.MessageLogged += context =>
+        {
+
+            if (context.EventId.Name == "Http2ConnectionClosed")
+            {
+                connectionClosed.SetResult();
+            }
+            else if (context.EventId.Name == "ConnectionReadFin")
+            {
+                readFin.SetResult();
+            }
+            else if (context.EventId.Name == "ConnectionWriteFin")
+            {
+                writeFin.SetResult();
+            }
+        };
+
+        var testContext = new TestServiceContext(LoggerFactory);
+
+        testContext.InitializeHeartbeat();
+
+        await using (var server = new TestServer(context =>
+        {
+            return context.Response.WriteAsync("hello world " + context.Request.Protocol);
+        },
+        testContext,
+        kestrelOptions =>
+        {
+            kestrelOptions.Listen(IPAddress.Loopback, 0, listenOptions =>
+            {
+                listenOptions.Protocols = HttpProtocols.Http2;
+                listenOptions.UseHttps(_x509Certificate2);
+            });
+        }))
+        {
+            var response = await Client.GetStringAsync($"https://localhost:{server.Port}/");
+            Assert.Equal("hello world HTTP/2", response);
+            Client.Dispose(); // Close the socket, no GoAway is sent.
+
+            await readFin.Task.DefaultTimeout();
+            await writeFin.Task.DefaultTimeout();
+            await connectionClosed.Task.DefaultTimeout();
+
+            await server.StopAsync();
+        }
+    }
+
     [CollectDump]
     [ConditionalFact]
     public async Task GracefulShutdownWaitsForRequestsToFinish()