Fix the HttpSys client disconnect race condition #12194 (#35401)

Author: Tratcher

src/Servers/HttpSys/HttpSysServer.slnf

@@ -3,6 +3,7 @@
     "path": "..\\..\\..\\AspNetCore.sln",
     "projects": [
       "src\\DefaultBuilder\\src\\Microsoft.AspNetCore.csproj",
+      "src\\Extensions\\Features\\src\\Microsoft.Extensions.Features.csproj",
       "src\\Hosting\\Abstractions\\src\\Microsoft.AspNetCore.Hosting.Abstractions.csproj",
       "src\\Hosting\\Hosting\\src\\Microsoft.AspNetCore.Hosting.csproj",
       "src\\Hosting\\Server.Abstractions\\src\\Microsoft.AspNetCore.Hosting.Server.Abstractions.csproj",
@@ -11,12 +12,18 @@
       "src\\Http\\Headers\\src\\Microsoft.Net.Http.Headers.csproj",
       "src\\Http\\Http.Abstractions\\src\\Microsoft.AspNetCore.Http.Abstractions.csproj",
       "src\\Http\\Http.Extensions\\src\\Microsoft.AspNetCore.Http.Extensions.csproj",
-      "src\\Extensions\\Features\\src\\Microsoft.Extensions.Features.csproj",
       "src\\Http\\Http.Features\\src\\Microsoft.AspNetCore.Http.Features.csproj",
       "src\\Http\\Http\\src\\Microsoft.AspNetCore.Http.csproj",
       "src\\Http\\Metadata\\src\\Microsoft.AspNetCore.Metadata.csproj",
+      "src\\Http\\Routing.Abstractions\\src\\Microsoft.AspNetCore.Routing.Abstractions.csproj",
+      "src\\Http\\Routing\\src\\Microsoft.AspNetCore.Routing.csproj",
       "src\\Http\\WebUtilities\\src\\Microsoft.AspNetCore.WebUtilities.csproj",
+      "src\\Middleware\\Diagnostics.Abstractions\\src\\Microsoft.AspNetCore.Diagnostics.Abstractions.csproj",
+      "src\\Middleware\\Diagnostics\\src\\Microsoft.AspNetCore.Diagnostics.csproj",
       "src\\Middleware\\HostFiltering\\src\\Microsoft.AspNetCore.HostFiltering.csproj",
+      "src\\Middleware\\HttpOverrides\\src\\Microsoft.AspNetCore.HttpOverrides.csproj",
+      "src\\ObjectPool\\src\\Microsoft.Extensions.ObjectPool.csproj",
+      "src\\Security\\Authorization\\Core\\src\\Microsoft.AspNetCore.Authorization.csproj",
       "src\\Servers\\Connections.Abstractions\\src\\Microsoft.AspNetCore.Connections.Abstractions.csproj",
       "src\\Servers\\HttpSys\\samples\\HotAddSample\\HotAddSample.csproj",
       "src\\Servers\\HttpSys\\samples\\QueueSharing\\QueueSharing.csproj",
@@ -25,7 +32,11 @@
       "src\\Servers\\HttpSys\\src\\Microsoft.AspNetCore.Server.HttpSys.csproj",
       "src\\Servers\\HttpSys\\test\\FunctionalTests\\Microsoft.AspNetCore.Server.HttpSys.FunctionalTests.csproj",
       "src\\Servers\\HttpSys\\test\\Tests\\Microsoft.AspNetCore.Server.HttpSys.Tests.csproj",
+      "src\\Servers\\IIS\\IISIntegration\\src\\Microsoft.AspNetCore.Server.IISIntegration.csproj",
       "src\\Servers\\IIS\\IIS\\src\\Microsoft.AspNetCore.Server.IIS.csproj",
+      "src\\Servers\\Kestrel\\Core\\src\\Microsoft.AspNetCore.Server.Kestrel.Core.csproj",
+      "src\\Servers\\Kestrel\\Kestrel\\src\\Microsoft.AspNetCore.Server.Kestrel.csproj",
+      "src\\Servers\\Kestrel\\Transport.Quic\\src\\Microsoft.AspNetCore.Server.Kestrel.Transport.Quic.csproj",
       "src\\Servers\\Kestrel\\Transport.Sockets\\src\\Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.csproj",
       "src\\Testing\\src\\Microsoft.AspNetCore.Testing.csproj"
     ]

src/Servers/HttpSys/src/RequestProcessing/RequestStreamAsyncResult.cs

@@ -135,6 +135,9 @@ internal void Complete(int read, uint errorCode = UnsafeNclNativeMethods.ErrorCo
 
         internal void Fail(Exception ex)
         {
+            // Make sure the Abort state is set before signaling the callback so we can avoid race condtions with user code.
+            Dispose();
+            _requestStream.Abort();
             if (_tcs.TrySetException(ex) && _callback != null)
             {
                 try
@@ -147,8 +150,6 @@ internal void Fail(Exception ex)
                     // TODO: Log
                 }
             }
-            Dispose();
-            _requestStream.Abort();
         }
 
         [SuppressMessage("Microsoft.Usage", "CA2216:DisposableTypesShouldDeclareFinalizer", Justification = "The disposable resource referenced does have a finalizer.")]

src/Servers/HttpSys/src/RequestProcessing/Response.cs

@@ -26,6 +26,7 @@ internal sealed class Response
         private static bool SupportsGoAway = true;
 
         private ResponseState _responseState;
+        private bool _aborted;
         private string? _reasonPhrase;
         private ResponseBody? _nativeStream;
         private AuthenticationSchemes _authChallenges;
@@ -196,14 +197,16 @@ internal void MakeTrailersReadOnly()
 
         internal void Abort()
         {
-            // Update state for HasStarted. Do not attempt a graceful Dispose.
-            _responseState = ResponseState.Closed;
+            // Do not attempt a graceful Dispose.
+            // _responseState is not modified because that refers to app state like modifying
+            // status and headers. See https://github.com/dotnet/aspnetcore/issues/12194.
+            _aborted = true;
         }
 
         // should only be called from RequestContext
         internal void Dispose()
         {
-            if (_responseState >= ResponseState.Closed)
+            if (_aborted || _responseState >= ResponseState.Closed)
             {
                 return;
             }

src/Servers/HttpSys/test/FunctionalTests/ResponseTests.cs

@@ -5,8 +5,11 @@
 using System.IO;
 using System.Net;
 using System.Net.Http;
+using System.Net.Sockets;
+using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Testing;
 using Xunit;
@@ -210,6 +213,68 @@ public async Task Response_OnStartingThrowsAfterWrite_WriteThrowsAndStillCallsOn
             }
         }
 
+        [ConditionalFact]
+        public async Task ClientDisconnectsBeforeResponse_ResponseCanStillBeModified()
+        {
+            var readStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+            var readCompleted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+            using var server = Utilities.CreateHttpServer(out var address, async httpContext =>
+            {
+                var readTask = httpContext.Request.Body.ReadAsync(new byte[10]);
+                readStarted.SetResult();
+                try
+                {
+                    await readTask;
+                    readCompleted.SetException(new InvalidOperationException("The read wasn't supposed to succeed"));
+                    return;
+                }
+                catch (IOException)
+                {
+                }
+
+                try
+                {
+                    // https://github.com/dotnet/aspnetcore/issues/12194
+                    // Modifying the response after the client has disconnected must be allowed.
+                    Assert.False(httpContext.Response.HasStarted);
+                    httpContext.Response.StatusCode = 400;
+                    httpContext.Response.ContentType = "text/plain";
+                    await httpContext.Response.WriteAsync("Body");
+                }
+                catch (Exception ex)
+                {
+                    readCompleted.SetException(ex);
+                    return;
+                }
+
+                readCompleted.SetResult();
+            });
+
+            // Send a request without the body.
+            var uri = new Uri(address);
+            StringBuilder builder = new StringBuilder();
+            builder.AppendLine("POST / HTTP/1.1");
+            builder.AppendLine("Connection: close");
+            builder.Append("HOST: ");
+            builder.AppendLine(uri.Authority);
+            builder.AppendLine("Content-Length: 10");
+            builder.AppendLine();
+
+            byte[] request = Encoding.ASCII.GetBytes(builder.ToString());
+
+            using var socket = new Socket(SocketType.Stream, ProtocolType.Tcp);
+            socket.Connect(uri.Host, uri.Port);
+            socket.Send(request);
+
+            await readStarted.Task.DefaultTimeout();
+
+            // Disconnect
+            socket.Close();
+
+            // Make sure the server code behaved as expected.
+            await readCompleted.Task.DefaultTimeout();
+        }
+
         private async Task<HttpResponseMessage> SendRequestAsync(string uri)
         {
             using (var client = new HttpClient())