Merged PR 32090: [7.0] Forcibly close socket on abort (#49927)

Forcibly close socket when connection is aborted. Co-authored-by: Andrew Casey <[email protected]>
dotnet · Aug 8, 2023 · acfe811 · acfe811
1 parent 42d7c23
commit acfe811
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
@@ -538,8  538,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
@@ -95,7  95,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
@@ -226,7  226,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
@@ -161,7  161,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
@@ -27,10  27,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
@@ -29,6  29,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,
@@ -37,7  38,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);
@@ -48,6  50,7 @@ internal SocketConnection(Socket socket,
         _logger = logger;
         _waitForData = waitForData;
         _socketSenderPool = socketSenderPool;
         _finOnError = finOnError;
 
         LocalEndPoint = _socket.LocalEndPoint;
         RemoteEndPoint = _socket.RemoteEndPoint;
@@ -376,11  379,21 @@ private void Shutdown(Exception? shutdownReason)
             // 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.");
 
             // 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
@@ -96,7  96,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()