From 29b7c5c0301d1e535d9c634bf9a779c1e58c305f Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Thu, 17 Jan 2019 09:38:04 -0800 Subject: [PATCH] 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 | 20 +---------- 3 files changed, 21 insertions(+), 36 deletions(-) diff --git a/eng/PatchConfig.props b/eng/PatchConfig.props index 5076e60f99..b1b7e01318 100644 --- a/eng/PatchConfig.props +++ b/eng/PatchConfig.props @@ -26,7 +26,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 e9b1d926aa..9d8dad0304 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -367,8 +367,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; @@ -412,16 +415,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); } } @@ -435,12 +437,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); } } @@ -460,6 +462,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 96e230fd64..f5baf45f12 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,20 +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)); - - Assert.Equal(originalToken, originalRegistration.Token); - } - [Fact] public async Task ExceptionDetailNotIncludedWhenLogLevelInformationNotEnabled() {