From 9f956161ffa33ea707c39986269382a061924b8b Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 12 Apr 2019 08:18:34 -0700 Subject: [PATCH 01/11] fixup --- .../Core/src/Internal/Http/HttpProtocol.cs | 45 ++++++++++++------- .../Kestrel/Core/test/Http1ConnectionTests.cs | 14 ++++++ 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index 0503c549305b..0db90b4eff5f 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -35,7 +35,7 @@ internal abstract partial class HttpProtocol : IDefaultHttpContextContainer, IHt private Stack, object>> _onCompleted; private object _abortLock = new object(); - private volatile bool _requestAborted; + private volatile bool _connectionAborted; private bool _preventRequestAbortedCancellation; private CancellationTokenSource _abortedCts; private CancellationToken? _manuallySetRequestAbortToken; @@ -257,7 +257,7 @@ public CancellationToken RequestAborted return new CancellationToken(false); } - if (_requestAborted) + if (_connectionAborted) { return new CancellationToken(true); } @@ -378,15 +378,18 @@ public void Reset() _preventRequestAbortedCancellation = false; // Lock to prevent CancelRequestAbortedToken from attempting to cancel an disposed CTS. + CancellationTokenSource cts = null; lock (_abortLock) { - if (!_requestAborted) + if (_abortedCts != null) { - _abortedCts?.Dispose(); + cts = _abortedCts; _abortedCts = null; } } + cts?.Dispose(); + if (_wrapperObjectsToDispose != null) { foreach (var disposable in _wrapperObjectsToDispose) @@ -442,9 +445,17 @@ private void CancelRequestAbortedToken() { try { - _abortedCts.Cancel(); - _abortedCts.Dispose(); - _abortedCts = null; + CancellationTokenSource cts = null; + lock (_abortLock) + { + if (_abortedCts != null) + { + cts = _abortedCts; + _abortedCts = null; + } + } + + cts?.Cancel(); } catch (Exception ex) { @@ -456,15 +467,17 @@ protected void AbortRequest() { lock (_abortLock) { - if (_requestAborted) + if (_connectionAborted) { return; } - _requestAborted = true; + _connectionAborted = true; } - if (_abortedCts != null) + // The check to _hasAborted is an optimization to not schedule a cancellation if the context is already disposed. + // In CancelRequestAbortedToken, we will relock to check again. + if (_abortedCts != null && !_preventRequestAbortedCancellation) { // Potentially calling user code. CancelRequestAbortedToken logs any exceptions. ServiceContext.Scheduler.Schedule(state => ((HttpProtocol)state).CancelRequestAbortedToken(), this); @@ -481,14 +494,12 @@ private void PreventRequestAbortedCancellation() { lock (_abortLock) { - if (_requestAborted) + if (_connectionAborted) { return; } _preventRequestAbortedCancellation = true; - _abortedCts?.Dispose(); - _abortedCts = null; } } @@ -594,7 +605,7 @@ private async Task ProcessRequests(IHttpApplication applicat // Run the application code for this request await application.ProcessRequestAsync(context); - if (!_requestAborted) + if (!_connectionAborted) { VerifyResponseContentLength(); } @@ -630,7 +641,7 @@ private async Task ProcessRequests(IHttpApplication applicat // 4XX responses are written by TryProduceInvalidRequestResponse during connection tear down. if (_requestRejectedException == null) { - if (!_requestAborted) + if (!_connectionAborted) { // Call ProduceEnd() before consuming the rest of the request body to prevent // delaying clients waiting for the chunk terminator: @@ -662,7 +673,7 @@ private async Task ProcessRequests(IHttpApplication applicat application.DisposeContext(context, _applicationException); // Even for non-keep-alive requests, try to consume the entire body to avoid RSTs. - if (!_requestAborted && _requestRejectedException == null && !messageBody.IsEmpty) + if (!_connectionAborted && _requestRejectedException == null && !messageBody.IsEmpty) { await messageBody.ConsumeAsync(); } @@ -964,7 +975,7 @@ private void VerifyInitializeState(int firstWriteByteCount) protected Task TryProduceInvalidRequestResponse() { // If _requestAborted is set, the connection has already been closed. - if (_requestRejectedException != null && !_requestAborted) + if (_requestRejectedException != null && !_connectionAborted) { return ProduceEnd(); } diff --git a/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs b/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs index ce1f86e7a6d5..6ac94c384a70 100644 --- a/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs +++ b/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs @@ -729,6 +729,20 @@ public async Task RequestAbortedTokenIsResetBeforeLastWriteWithChunkedEncoding() 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 void RequestAbortedTokenIsUsableAfterCancellation() { From d22e25615d2ddb79d953dfcb73b0f87f4cc08994 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 12 Apr 2019 09:09:49 -0700 Subject: [PATCH 02/11] Update a bit of logic --- .../Kestrel/Core/src/Internal/Http/HttpProtocol.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index 0db90b4eff5f..4e91745ff341 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -450,11 +450,20 @@ private void CancelRequestAbortedToken() { if (_abortedCts != null) { - cts = _abortedCts; + if (!_preventRequestAbortedCancellation) + { + cts = _abortedCts; + } _abortedCts = null; } } + // There is technically a race here where _preventRequestAbortCancellation + // changes to true right here and then we call cts.Cancel. + // However, this race already existed, where the cts was canceled just before + // the _preventRequestAbortedCancellation is set to true. + // _preventRequestAbortedCancellation is mostly a heuristic rather than a guarantee + // we need to follow. cts?.Cancel(); } catch (Exception ex) @@ -475,8 +484,6 @@ protected void AbortRequest() _connectionAborted = true; } - // The check to _hasAborted is an optimization to not schedule a cancellation if the context is already disposed. - // In CancelRequestAbortedToken, we will relock to check again. if (_abortedCts != null && !_preventRequestAbortedCancellation) { // Potentially calling user code. CancelRequestAbortedToken logs any exceptions. From 79557f7541a40da5142eda1fbbdb936c27509c39 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 12 Apr 2019 09:25:12 -0700 Subject: [PATCH 03/11] temp --- .../Kestrel/Core/src/Internal/Http/HttpProtocol.cs | 12 ++++++------ .../test/InMemory.FunctionalTests/HttpsTests.cs | 1 + 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index 4e91745ff341..0b531ce8b874 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -378,17 +378,17 @@ public void Reset() _preventRequestAbortedCancellation = false; // Lock to prevent CancelRequestAbortedToken from attempting to cancel an disposed CTS. - CancellationTokenSource cts = null; + CancellationTokenSource localAbortCts = null; lock (_abortLock) { if (_abortedCts != null) { - cts = _abortedCts; + localAbortCts = _abortedCts; _abortedCts = null; } } - cts?.Dispose(); + localAbortCts?.Dispose(); if (_wrapperObjectsToDispose != null) { @@ -445,14 +445,14 @@ private void CancelRequestAbortedToken() { try { - CancellationTokenSource cts = null; + CancellationTokenSource localAbortCts = null; lock (_abortLock) { if (_abortedCts != null) { if (!_preventRequestAbortedCancellation) { - cts = _abortedCts; + localAbortCts = _abortedCts; } _abortedCts = null; } @@ -464,7 +464,7 @@ private void CancelRequestAbortedToken() // the _preventRequestAbortedCancellation is set to true. // _preventRequestAbortedCancellation is mostly a heuristic rather than a guarantee // we need to follow. - cts?.Cancel(); + localAbortCts?.Cancel(); } catch (Exception ex) { diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs index 6f834ebc8ccd..d0093f1bd391 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs @@ -217,6 +217,7 @@ await sslStream.AuthenticateAsClientAsync("127.0.0.1", clientCertificates: null, } [Fact] + //[Repeat] public async Task DoesNotThrowObjectDisposedExceptionFromWriteAsyncAfterConnectionIsAborted() { var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); From 461a9e2b82a3aae8998912031f808a42f368ae53 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 12 Apr 2019 09:57:14 -0700 Subject: [PATCH 04/11] Repeat test that caused deadlock --- src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs index d0093f1bd391..efd1d567223a 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs @@ -217,7 +217,7 @@ await sslStream.AuthenticateAsClientAsync("127.0.0.1", clientCertificates: null, } [Fact] - //[Repeat] + [Repeat(20)] public async Task DoesNotThrowObjectDisposedExceptionFromWriteAsyncAfterConnectionIsAborted() { var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); From 2847089ba5a939dad5158bd8227fe34c5684062e Mon Sep 17 00:00:00 2001 From: Chris Ross Date: Fri, 12 Apr 2019 11:49:51 -0700 Subject: [PATCH 05/11] Update src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs Co-Authored-By: jkotalik --- src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index 0b531ce8b874..0d7ba2d877ca 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -377,7 +377,7 @@ public void Reset() _manuallySetRequestAbortToken = null; _preventRequestAbortedCancellation = false; - // Lock to prevent CancelRequestAbortedToken from attempting to cancel an disposed CTS. + // Lock to prevent CancelRequestAbortedToken from attempting to cancel a disposed CTS. CancellationTokenSource localAbortCts = null; lock (_abortLock) { From 6ef3008295c28414902a600e13675038eebb91a5 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 12 Apr 2019 12:21:28 -0700 Subject: [PATCH 06/11] Feedback --- .../Core/src/Internal/Http/HttpProtocol.cs | 37 +++++++++++++------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index 0d7ba2d877ca..42a833dd3987 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -375,12 +375,12 @@ public void Reset() Scheme = _scheme; _manuallySetRequestAbortToken = null; - _preventRequestAbortedCancellation = false; // Lock to prevent CancelRequestAbortedToken from attempting to cancel a disposed CTS. CancellationTokenSource localAbortCts = null; lock (_abortLock) { + _preventRequestAbortedCancellation = false; if (_abortedCts != null) { localAbortCts = _abortedCts; @@ -446,25 +446,36 @@ private void CancelRequestAbortedToken() try { CancellationTokenSource localAbortCts = null; + var shouldDispose = false; + var shouldCancel = false; lock (_abortLock) { if (_abortedCts != null) { - if (!_preventRequestAbortedCancellation) + localAbortCts = _abortedCts; + _abortedCts = null; + + if (_preventRequestAbortedCancellation) { - localAbortCts = _abortedCts; + shouldDispose = true; + } + else + { + shouldCancel = true; } - _abortedCts = null; } } - // There is technically a race here where _preventRequestAbortCancellation - // changes to true right here and then we call cts.Cancel. - // However, this race already existed, where the cts was canceled just before - // the _preventRequestAbortedCancellation is set to true. - // _preventRequestAbortedCancellation is mostly a heuristic rather than a guarantee - // we need to follow. - localAbortCts?.Cancel(); + // If we cancel the cts, we don't dispose as people may still be using + // the cts. It also isn't necessary to dispose a canceled cts. + if (shouldDispose) + { + localAbortCts.Dispose(); + } + else if (shouldCancel) + { + localAbortCts.Cancel(); + } } catch (Exception ex) { @@ -474,6 +485,7 @@ private void CancelRequestAbortedToken() protected void AbortRequest() { + var shouldScheduleCancellation = false; lock (_abortLock) { if (_connectionAborted) @@ -481,10 +493,11 @@ protected void AbortRequest() return; } + shouldScheduleCancellation = _abortedCts != null && !_preventRequestAbortedCancellation; _connectionAborted = true; } - if (_abortedCts != null && !_preventRequestAbortedCancellation) + if (shouldScheduleCancellation) { // Potentially calling user code. CancelRequestAbortedToken logs any exceptions. ServiceContext.Scheduler.Schedule(state => ((HttpProtocol)state).CancelRequestAbortedToken(), this); From 9068b7198c0bcc103020c0c0df8eb44ae5046b64 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 12 Apr 2019 12:30:56 -0700 Subject: [PATCH 07/11] Don't call dispose in CancelRequestAbortedToken --- .../Core/src/Internal/Http/HttpProtocol.cs | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index 42a833dd3987..e5d49d883095 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -446,33 +446,20 @@ private void CancelRequestAbortedToken() try { CancellationTokenSource localAbortCts = null; - var shouldDispose = false; var shouldCancel = false; lock (_abortLock) { - if (_abortedCts != null) + if (_abortedCts != null && !_preventRequestAbortedCancellation) { localAbortCts = _abortedCts; _abortedCts = null; - - if (_preventRequestAbortedCancellation) - { - shouldDispose = true; - } - else - { - shouldCancel = true; - } + shouldCancel = true; } } // If we cancel the cts, we don't dispose as people may still be using // the cts. It also isn't necessary to dispose a canceled cts. - if (shouldDispose) - { - localAbortCts.Dispose(); - } - else if (shouldCancel) + if (shouldCancel) { localAbortCts.Cancel(); } From c3b28569f103db7fb7c2c898922dbc48182a9804 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 12 Apr 2019 12:44:19 -0700 Subject: [PATCH 08/11] Final round --- src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index e5d49d883095..c32dbaf54b86 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -378,6 +378,7 @@ public void Reset() // Lock to prevent CancelRequestAbortedToken from attempting to cancel a disposed CTS. CancellationTokenSource localAbortCts = null; + lock (_abortLock) { _preventRequestAbortedCancellation = false; @@ -446,20 +447,19 @@ private void CancelRequestAbortedToken() try { CancellationTokenSource localAbortCts = null; - var shouldCancel = false; + lock (_abortLock) { if (_abortedCts != null && !_preventRequestAbortedCancellation) { localAbortCts = _abortedCts; _abortedCts = null; - shouldCancel = true; } } // If we cancel the cts, we don't dispose as people may still be using // the cts. It also isn't necessary to dispose a canceled cts. - if (shouldCancel) + if (localAbortCts != null) { localAbortCts.Cancel(); } @@ -473,6 +473,7 @@ private void CancelRequestAbortedToken() protected void AbortRequest() { var shouldScheduleCancellation = false; + lock (_abortLock) { if (_connectionAborted) From f5865cd8d3a5c61729e9e964fede12b38ae8a4eb Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 12 Apr 2019 12:54:47 -0700 Subject: [PATCH 09/11] Update HttpProtocol.cs --- src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index c32dbaf54b86..201175c987fe 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -459,10 +459,7 @@ private void CancelRequestAbortedToken() // If we cancel the cts, we don't dispose as people may still be using // the cts. It also isn't necessary to dispose a canceled cts. - if (localAbortCts != null) - { - localAbortCts.Cancel(); - } + localAbortedCts?.Cancel() } catch (Exception ex) { From 6be0c9825efd9b4f5658c8f85a3a02ed6cd9d2cd Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 12 Apr 2019 14:56:32 -0700 Subject: [PATCH 10/11] Update HttpProtocol.cs --- .../Kestrel/Core/src/Internal/Http/HttpProtocol.cs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index 201175c987fe..44ff28394c40 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -382,11 +382,8 @@ public void Reset() lock (_abortLock) { _preventRequestAbortedCancellation = false; - if (_abortedCts != null) - { - localAbortCts = _abortedCts; - _abortedCts = null; - } + localAbortCts = _abortedCts; + _abortedCts = null; } localAbortCts?.Dispose(); @@ -459,7 +456,7 @@ private void CancelRequestAbortedToken() // If we cancel the cts, we don't dispose as people may still be using // the cts. It also isn't necessary to dispose a canceled cts. - localAbortedCts?.Cancel() + localAbortedCts?.Cancel(); } catch (Exception ex) { From 5619a7e0de6e45c58fb53c39c35a46fd7b7f59e3 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Fri, 12 Apr 2019 15:04:49 -0700 Subject: [PATCH 11/11] Update HttpProtocol.cs --- src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index 44ff28394c40..8131508a82a1 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -456,7 +456,7 @@ private void CancelRequestAbortedToken() // If we cancel the cts, we don't dispose as people may still be using // the cts. It also isn't necessary to dispose a canceled cts. - localAbortedCts?.Cancel(); + localAbortCts?.Cancel(); } catch (Exception ex) {