Merge pull request #6994 from aspnet/halter73/1531-part2

Revert "Wait to dispose RequestAborted CTS (#4447)"

eng/PatchConfig.props

@@ -36,7 +36,6 @@ Later on, this will be checked using this condition:
       Microsoft.AspNetCore.Mvc.Core;
       Microsoft.AspNetCore.Routing;
       Microsoft.AspNetCore.Server.IIS;
-      Microsoft.AspNetCore.Server.Kestrel.Core;
       java:signalr;
     </PackagesInPatch>
   </PropertyGroup>

src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

@@ -348,8 +348,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
             // Lock to prevent CancelRequestAbortedToken from attempting to cancel an disposed CTS.
             lock (_abortLock)
             {
-                _abortedCts?.Dispose();
-                _abortedCts = null;
+                if (!_requestAborted)
+                {
+                    _abortedCts?.Dispose();
+                    _abortedCts = null;
+                }
             }
 
             _requestHeadersParsed = 0;
@@ -391,16 +394,15 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
 
         private void CancelRequestAbortedToken()
         {
-            lock (_abortLock)
+            try
             {
-                try
-                {
-                    _abortedCts?.Cancel();
-                }
-                catch (Exception ex)
-                {
-                    Log.ApplicationError(ConnectionId, TraceIdentifier, ex);
-                }
+                _abortedCts.Cancel();
+                _abortedCts.Dispose();
+                _abortedCts = null;
+            }
+            catch (Exception ex)
+            {
+                Log.ApplicationError(ConnectionId, TraceIdentifier, ex);
             }
         }
 
@@ -414,12 +416,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
                 }
 
                 _requestAborted = true;
+            }
 
-                if (_abortedCts != null && !_preventRequestAbortedCancellation)
-                {
-                    // Potentially calling user code. CancelRequestAbortedToken logs any exceptions.
-                    ServiceContext.Scheduler.Schedule(state => ((HttpProtocol)state).CancelRequestAbortedToken(), this);
-                }
+            if (_abortedCts != null)
+            {
+                // Potentially calling user code. CancelRequestAbortedToken logs any exceptions.
+                ServiceContext.Scheduler.Schedule(state => ((HttpProtocol)state).CancelRequestAbortedToken(), this);
             }
         }
 
@@ -439,6 +441,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
                 }
 
                 _preventRequestAbortedCancellation = true;
+                _abortedCts?.Dispose();
+                _abortedCts = null;
             }
         }
 

src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs

@@ -729,14 +729,16 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests
         }
 
         [Fact]
-        public void RequestAbortedTokenIsFullyUsableAfterCancellation()
+        public void RequestAbortedTokenIsUsableAfterCancellation()
         {
             var originalToken = _http1Connection.RequestAborted;
             var originalRegistration = originalToken.Register(() => { });
 
             _http1Connection.Abort(new ConnectionAbortedException());
 
-            Assert.True(originalToken.WaitHandle.WaitOne(TestConstants.DefaultTimeout));
+            // The following line will throw an ODE because the original CTS backing the token has been diposed.
+            // See https://github.com/aspnet/AspNetCore/pull/4447 for the history behind this test.
+            //Assert.True(originalToken.WaitHandle.WaitOne(TestConstants.DefaultTimeout));
             Assert.True(_http1Connection.RequestAborted.WaitHandle.WaitOne(TestConstants.DefaultTimeout));
 
 #if NETCOREAPP2_2