From 35db355d2ad564698de9661d8c17a3d8764b88dd Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 9 Jun 2021 14:00:38 -0700 Subject: [PATCH 1/3] Stop incorrect TaskCanceledExceptions from HttpRequestPipeReader --- .../Internal/Http/HttpRequestPipeReader.cs | 47 +++++++++---------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs index e53efe34a45d..17f6a1ebec41 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs @@ -93,44 +93,43 @@ public void StopAcceptingReads() public void Abort(Exception? error = null) { - // We don't want to throw an ODE until the app func actually completes. // If the request is aborted, we throw a TaskCanceledException instead, // unless error is not null, in which case we throw it. - if (_state != HttpStreamState.Closed) + + if (error is object) + { + _error ??= ExceptionDispatchInfo.Capture(error); + } + else { + // Do not change state if there is no error because we don't want to throw a TaskCanceledException + // and we do not want to introduce any memory barriers at this layer. This is just for reporting errors + // early when we know the transport will fail. _state = HttpStreamState.Aborted; - - if (error is object && _error is null) - { - _error = ExceptionDispatchInfo.Capture(error); - } } } [MethodImpl(MethodImplOptions.AggressiveInlining)] private void ValidateState(CancellationToken cancellationToken = default) { - var state = _state; - if (state == HttpStreamState.Open) + switch (_state) { - cancellationToken.ThrowIfCancellationRequested(); - } - else if (state == HttpStreamState.Closed) - { - ThrowObjectDisposedException(); - } - else - { - if (_error is object) - { - _error.Throw(); - } - else - { + case HttpStreamState.Open: + cancellationToken.ThrowIfCancellationRequested(); + break; + case HttpStreamState.Closed: + ThrowObjectDisposedException(); + break; + case HttpStreamState.Aborted: ThrowTaskCanceledException(); - } + break; } + // Abort errors are always terminal. We don't use _state to see if there is an error + // because we don't want to be forced to synchronize. This is best effort. The transport should + // report the same error we aborted it with if the read gets that far. + _error?.Throw(); + static void ThrowObjectDisposedException() => throw new ObjectDisposedException(nameof(HttpRequestStream)); static void ThrowTaskCanceledException() => throw new TaskCanceledException(); } From 55c31c1690087ad156521b951f8276ff7a45e5a9 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 9 Jun 2021 14:31:12 -0700 Subject: [PATCH 2/3] Update src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs Co-authored-by: James Newton-King --- .../Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs index 17f6a1ebec41..c6eac45e13ae 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs @@ -96,7 +96,7 @@ public void Abort(Exception? error = null) // If the request is aborted, we throw a TaskCanceledException instead, // unless error is not null, in which case we throw it. - if (error is object) + if (error is not null) { _error ??= ExceptionDispatchInfo.Capture(error); } From 0fa1ae2f5511a287410f57372af057930f4f3c63 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 9 Jun 2021 14:38:25 -0700 Subject: [PATCH 3/3] Fix comment --- .../Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs index c6eac45e13ae..e9f0586e60ef 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs @@ -102,7 +102,7 @@ public void Abort(Exception? error = null) } else { - // Do not change state if there is no error because we don't want to throw a TaskCanceledException + // Do not change state if there is an error because we don't want to throw a TaskCanceledException // and we do not want to introduce any memory barriers at this layer. This is just for reporting errors // early when we know the transport will fail. _state = HttpStreamState.Aborted;