Skip to content

Commit 5d29560

Browse files
authored
Avoid sending EndStream after RST_STREAM with dedicated lock (#54552)
A race condition between sending RST_STREAM and checking conditions for sending EndStream was discovered during stress testing. It happens to be possible that in time after Http2Stream [checked the _responseCompletionState](https://github.com/dotnet/runtime/blob/183c4d100f68fb6c177a1fe71809d581aa25e47b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs#L275) and [actually send EndStream](https://github.com/dotnet/runtime/blob/183c4d100f68fb6c177a1fe71809d581aa25e47b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs#L288), a concurrent call to [Cancel method sends a RST_STREAM frame](https://github.com/dotnet/runtime/blob/183c4d100f68fb6c177a1fe71809d581aa25e47b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs#L389). Such reordering is disallowed by HTTP/2 protocol. Note: The issue and fix were verified manually under the debugger because currently it's not clear how to reliably simulate that situation. Fixes #42200
1 parent 5f8a177 commit 5d29560

File tree

1 file changed

+20
-14
lines changed
  • src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler

1 file changed

+20
-14
lines changed

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -245,9 +245,10 @@ public async Task SendRequestBodyAsync(CancellationToken cancellationToken)
245245

246246
_requestCompletionState = StreamCompletionState.Failed;
247247
Complete();
248+
249+
SendReset();
248250
}
249251

250-
SendReset();
251252
if (signalWaiter)
252253
{
253254
_waitSource.SetResult(true);
@@ -275,17 +276,17 @@ public async Task SendRequestBodyAsync(CancellationToken cancellationToken)
275276
sendReset = _responseCompletionState == StreamCompletionState.Failed;
276277
Complete();
277278
}
278-
}
279279

280-
if (sendReset)
281-
{
282-
SendReset();
283-
}
284-
else
285-
{
286-
// Send EndStream asynchronously and without cancellation.
287-
// If this fails, it means that the connection is aborting and we will be reset.
288-
_connection.LogExceptions(_connection.SendEndStreamAsync(StreamId));
280+
if (sendReset)
281+
{
282+
SendReset();
283+
}
284+
else
285+
{
286+
// Send EndStream asynchronously and without cancellation.
287+
// If this fails, it means that the connection is aborting and we will be reset.
288+
_connection.LogExceptions(_connection.SendEndStreamAsync(StreamId));
289+
}
289290
}
290291
}
291292
}
@@ -325,7 +326,7 @@ public async ValueTask<bool> WaitFor100ContinueAsync(CancellationToken cancellat
325326

326327
private void SendReset()
327328
{
328-
Debug.Assert(!Monitor.IsEntered(SyncObject));
329+
Debug.Assert(Monitor.IsEntered(SyncObject));
329330
Debug.Assert(_requestCompletionState != StreamCompletionState.InProgress);
330331
Debug.Assert(_responseCompletionState != StreamCompletionState.InProgress);
331332
Debug.Assert(_requestCompletionState == StreamCompletionState.Failed || _responseCompletionState == StreamCompletionState.Failed,
@@ -336,6 +337,8 @@ private void SendReset()
336337
// Don't send a RST_STREAM if we've already received one from the server.
337338
if (_resetException == null)
338339
{
340+
// If execution reached this line, it's guaranteed that
341+
// _requestCompletionState == StreamCompletionState.Failed or _responseCompletionState == StreamCompletionState.Failed
339342
_connection.LogExceptions(_connection.SendRstStreamAsync(StreamId, Http2ProtocolErrorCode.Cancel));
340343
}
341344
}
@@ -384,9 +387,12 @@ private void Cancel()
384387
// When cancellation propagates, SendRequestBodyAsync will set _requestCompletionState to Failed
385388
requestBodyCancellationSource?.Cancel();
386389

387-
if (sendReset)
390+
lock (SyncObject)
388391
{
389-
SendReset();
392+
if (sendReset)
393+
{
394+
SendReset();
395+
}
390396
}
391397

392398
if (signalWaiter)

0 commit comments

Comments
 (0)