From 5d554aeecd1aec8bc422052801a95e05c2393aed Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 24 Jan 2019 11:23:58 -0800 Subject: [PATCH 1/2] Revert "Wait to dispose RequestAborted CTS (#4447)" This reverts commit 0622513058eff9a4374a0edca726d2c62959ea1e. --- eng/PatchConfig.props | 1 - .../Core/src/Internal/Http/HttpProtocol.cs | 36 ++++++++++--------- .../Kestrel/Core/test/Http1ConnectionTests.cs | 25 +------------ 3 files changed, 21 insertions(+), 41 deletions(-) diff --git a/eng/PatchConfig.props b/eng/PatchConfig.props index 98d068b407..7e408b0dcc 100644 --- a/eng/PatchConfig.props +++ b/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; diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index cf7ca09e9a..3309701e0a 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/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; } } diff --git a/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs b/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs index 70301631ed..d60303d625 100644 --- a/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs +++ b/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs @@ -53,11 +53,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var connectionFeatures = new FeatureCollection(); connectionFeatures.Set(Mock.Of()); - _serviceContext = new TestServiceContext() - { - Scheduler = PipeScheduler.Inline - }; - + _serviceContext = new TestServiceContext(); _timeoutControl = new Mock(); _http1ConnectionContext = new HttpConnectionContext { @@ -728,25 +724,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests Assert.False(_http1Connection.RequestAborted.IsCancellationRequested); } - [Fact] - public void RequestAbortedTokenIsFullyUsableAfterCancellation() - { - var originalToken = _http1Connection.RequestAborted; - var originalRegistration = originalToken.Register(() => { }); - - _http1Connection.Abort(new ConnectionAbortedException()); - - Assert.True(originalToken.WaitHandle.WaitOne(TestConstants.DefaultTimeout)); - Assert.True(_http1Connection.RequestAborted.WaitHandle.WaitOne(TestConstants.DefaultTimeout)); - -#if NETCOREAPP2_2 - Assert.Equal(originalToken, originalRegistration.Token); -#elif NET461 -#else -#error Target framework needs to be updated -#endif - } - [Fact] public async Task ExceptionDetailNotIncludedWhenLogLevelInformationNotEnabled() { From 2853b451a2e689a540f97349bdad7b2251c8d729 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 24 Jan 2019 12:38:16 -0800 Subject: [PATCH 2/2] Add RequestAbortedTokenIsUsableAfterCancellation test --- .../Kestrel/Core/test/Http1ConnectionTests.cs | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs b/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs index d60303d625..b8420c3d5e 100644 --- a/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs +++ b/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs @@ -53,7 +53,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var connectionFeatures = new FeatureCollection(); connectionFeatures.Set(Mock.Of()); - _serviceContext = new TestServiceContext(); + _serviceContext = new TestServiceContext() + { + Scheduler = PipeScheduler.Inline + }; + _timeoutControl = new Mock(); _http1ConnectionContext = new HttpConnectionContext { @@ -724,6 +728,27 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests Assert.False(_http1Connection.RequestAborted.IsCancellationRequested); } + [Fact] + public void RequestAbortedTokenIsUsableAfterCancellation() + { + var originalToken = _http1Connection.RequestAborted; + var originalRegistration = originalToken.Register(() => { }); + + _http1Connection.Abort(new ConnectionAbortedException()); + + // 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 + Assert.Equal(originalToken, originalRegistration.Token); +#elif NET461 +#else +#error Target framework needs to be updated +#endif + } + [Fact] public async Task ExceptionDetailNotIncludedWhenLogLevelInformationNotEnabled() {