From 0797e8a5d732a6fce8a65dd18271e6e482f72b4a Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 17 Jun 2025 18:03:19 -0700 Subject: [PATCH 1/3] Avoid race condition that can cause RequestAborted not to fire - With HTTP/1.1 there's a narrow race where if we cancel the _abortedCts on a background thread due to the underlying connection closing between requests, it can be missed because CTS.TryReset is not thread safe with concurrent cancellation https://github.com/dotnet/runtime/blob/2dbdff1a7aebd64b4b265d99b173cd77f088c36e/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs#L477-L479 --- .../Core/src/Internal/Http/HttpProtocol.cs | 16 +++++++--------- .../Core/src/Internal/Http2/Http2Stream.cs | 1 - .../Core/src/Internal/Http3/Http3Connection.cs | 7 ++++--- .../Core/src/Internal/Http3/Http3Stream.cs | 3 ++- 4 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index 664e466ccefb..7d3a3f3f8553 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -407,21 +407,19 @@ public void Reset() _manuallySetRequestAbortToken = null; - // Lock to prevent CancelRequestAbortedToken from attempting to cancel a disposed CTS. - CancellationTokenSource? localAbortCts = null; - lock (_abortLock) { _preventRequestAbortedCancellation = false; - if (_abortedCts?.TryReset() == false) + + // If the connection has already been aborted, allow that to be observed during the next request. + if (!_connectionAborted && _abortedCts is not null) { - localAbortCts = _abortedCts; - _abortedCts = null; + // _connectionAborted is terminal and only set inside the _abortLock, so if it isn't set here, + // it has not been canceled yet. + Trace.Assert(_abortedCts.TryReset()); } } - localAbortCts?.Dispose(); - Output?.Reset(); _requestHeadersParsed = 0; @@ -760,7 +758,7 @@ private async Task ProcessRequests(IHttpApplication applicat } else if (!HasResponseStarted) { - // If the request was aborted and no response was sent, we use status code 499 for logging + // If the request was aborted and no response was sent, we use status code 499 for logging StatusCode = StatusCodes.Status499ClientClosedRequest; } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs index da2b547c03a7..8524b35f3947 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs @@ -129,7 +129,6 @@ public bool ReceivedEmptyRequestBody protected override void OnReset() { _keepAlive = true; - _connectionAborted = false; _userTrailers = null; // Reset Http2 Features diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index 20ec37eb3cc7..3716c96065bc 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -602,15 +602,16 @@ private async Task CreateHttp3Stream(ConnectionContext streamContext, // Check whether there is an existing HTTP/3 stream on the transport stream. // A stream will only be cached if the transport stream itself is reused. - if (!persistentStateFeature.State.TryGetValue(StreamPersistentStateKey, out var s)) + if (!persistentStateFeature.State.TryGetValue(StreamPersistentStateKey, out var s) || + s is not Http3Stream { CanReuse: true } reusableStream) { stream = new Http3Stream(application, CreateHttpStreamContext(streamContext)); persistentStateFeature.State.Add(StreamPersistentStateKey, stream); } else { - stream = (Http3Stream)s!; - stream.InitializeWithExistingContext(streamContext.Transport); + reusableStream.InitializeWithExistingContext(streamContext.Transport); + stream = reusableStream; } _streamLifetimeHandler.OnStreamCreated(stream); diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs index 36c87cd8ef7b..758f9544b544 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs @@ -65,6 +65,8 @@ internal abstract partial class Http3Stream : HttpProtocol, IHttp3Stream, IHttpS private bool IsAbortedRead => (_completionState & StreamCompletionFlags.AbortedRead) == StreamCompletionFlags.AbortedRead; public bool IsCompleted => (_completionState & StreamCompletionFlags.Completed) == StreamCompletionFlags.Completed; + public bool CanReuse => !_connectionAborted && HasResponseCompleted; + public Pipe RequestBodyPipe { get; private set; } = default!; public long? InputRemaining { get; internal set; } public QPackDecoder QPackDecoder { get; private set; } = default!; @@ -941,7 +943,6 @@ private Task ProcessDataFrameAsync(in ReadOnlySequence payload) protected override void OnReset() { _keepAlive = true; - _connectionAborted = false; _userTrailers = null; _isWebTransportSessionAccepted = false; _isMethodConnect = false; From 121b387cd10240ae684c1f2786ce14daaf88182c Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 17 Jun 2025 19:03:42 -0700 Subject: [PATCH 2/3] Use Debug.Assert --- src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs | 5 +++-- .../Kestrel/Core/src/Internal/Http3/Http3Connection.cs | 2 +- 2 files 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 7d3a3f3f8553..094ba0058b87 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -415,8 +415,9 @@ public void Reset() if (!_connectionAborted && _abortedCts is not null) { // _connectionAborted is terminal and only set inside the _abortLock, so if it isn't set here, - // it has not been canceled yet. - Trace.Assert(_abortedCts.TryReset()); + // _abortedCts has not been canceled yet. + var resetSuccess = _abortedCts.TryReset(); + Debug.Assert(resetSuccess); } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index 3716c96065bc..fffd8e35463a 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -610,8 +610,8 @@ private async Task CreateHttp3Stream(ConnectionContext streamContext, } else { - reusableStream.InitializeWithExistingContext(streamContext.Transport); stream = reusableStream; + reusableStream.InitializeWithExistingContext(streamContext.Transport); } _streamLifetimeHandler.OnStreamCreated(stream); From 5630fc32f202a6cc683b8d558104e1a1836043f7 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 17 Jun 2025 19:13:56 -0700 Subject: [PATCH 3/3] Overwrite HTTP/3 stream in state dictionary if old one is not reusable --- src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index fffd8e35463a..c2db22acd8bd 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -606,7 +606,7 @@ private async Task CreateHttp3Stream(ConnectionContext streamContext, s is not Http3Stream { CanReuse: true } reusableStream) { stream = new Http3Stream(application, CreateHttpStreamContext(streamContext)); - persistentStateFeature.State.Add(StreamPersistentStateKey, stream); + persistentStateFeature.State[StreamPersistentStateKey] = stream; } else {