Skip to content

Commit 4d30367

Browse files
committed
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
1 parent 045d245 commit 4d30367

File tree

4 files changed

+13
-14
lines changed

4 files changed

+13
-14
lines changed

src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -407,21 +407,19 @@ public void Reset()
407407

408408
_manuallySetRequestAbortToken = null;
409409

410-
// Lock to prevent CancelRequestAbortedToken from attempting to cancel a disposed CTS.
411-
CancellationTokenSource? localAbortCts = null;
412-
413410
lock (_abortLock)
414411
{
415412
_preventRequestAbortedCancellation = false;
416-
if (_abortedCts?.TryReset() == false)
413+
414+
// If the connection has already been aborted, allow that to be observed during the next request.
415+
if (!_connectionAborted && _abortedCts is not null)
417416
{
418-
localAbortCts = _abortedCts;
419-
_abortedCts = null;
417+
// _connectionAborted is terminal and only set inside the _abortLock, so if it isn't set here,
418+
// it has not been canceled yet.
419+
Trace.Assert(_abortedCts.TryReset());
420420
}
421421
}
422422

423-
localAbortCts?.Dispose();
424-
425423
Output?.Reset();
426424

427425
_requestHeadersParsed = 0;
@@ -760,7 +758,7 @@ private async Task ProcessRequests<TContext>(IHttpApplication<TContext> applicat
760758
}
761759
else if (!HasResponseStarted)
762760
{
763-
// If the request was aborted and no response was sent, we use status code 499 for logging
761+
// If the request was aborted and no response was sent, we use status code 499 for logging
764762
StatusCode = StatusCodes.Status499ClientClosedRequest;
765763
}
766764
}

src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,6 @@ public bool ReceivedEmptyRequestBody
129129
protected override void OnReset()
130130
{
131131
_keepAlive = true;
132-
_connectionAborted = false;
133132
_userTrailers = null;
134133

135134
// Reset Http2 Features

src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -602,15 +602,16 @@ private async Task CreateHttp3Stream<TContext>(ConnectionContext streamContext,
602602

603603
// Check whether there is an existing HTTP/3 stream on the transport stream.
604604
// A stream will only be cached if the transport stream itself is reused.
605-
if (!persistentStateFeature.State.TryGetValue(StreamPersistentStateKey, out var s))
605+
if (!persistentStateFeature.State.TryGetValue(StreamPersistentStateKey, out var s) ||
606+
s is not Http3Stream<TContext> { CanReuse: true } reusableStream)
606607
{
607608
stream = new Http3Stream<TContext>(application, CreateHttpStreamContext(streamContext));
608609
persistentStateFeature.State.Add(StreamPersistentStateKey, stream);
609610
}
610611
else
611612
{
612-
stream = (Http3Stream<TContext>)s!;
613-
stream.InitializeWithExistingContext(streamContext.Transport);
613+
reusableStream.InitializeWithExistingContext(streamContext.Transport);
614+
stream = reusableStream;
614615
}
615616

616617
_streamLifetimeHandler.OnStreamCreated(stream);

src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,8 @@ internal abstract partial class Http3Stream : HttpProtocol, IHttp3Stream, IHttpS
6666
private bool IsAbortedRead => (_completionState & StreamCompletionFlags.AbortedRead) == StreamCompletionFlags.AbortedRead;
6767
public bool IsCompleted => (_completionState & StreamCompletionFlags.Completed) == StreamCompletionFlags.Completed;
6868

69+
public bool CanReuse => !_connectionAborted && HasResponseCompleted;
70+
6971
public bool ReceivedEmptyRequestBody
7072
{
7173
get
@@ -957,7 +959,6 @@ private Task ProcessDataFrameAsync(in ReadOnlySequence<byte> payload)
957959
protected override void OnReset()
958960
{
959961
_keepAlive = true;
960-
_connectionAborted = false;
961962
_userTrailers = null;
962963
_isWebTransportSessionAccepted = false;
963964
_isMethodConnect = false;

0 commit comments

Comments
 (0)