Skip to content

Commit

Permalink
Avoid sending EndStream after RST_STREAM with dedicated lock (dotnet#…
Browse files Browse the repository at this point in the history
…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 dotnet#42200
  • Loading branch information
alnikola authored Jun 29, 2021
1 parent 5f8a177 commit 5d29560
Showing 1 changed file with 20 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,10 @@ public async Task SendRequestBodyAsync(CancellationToken cancellationToken)

_requestCompletionState = StreamCompletionState.Failed;
Complete();

SendReset();
}

SendReset();
if (signalWaiter)
{
_waitSource.SetResult(true);
Expand Down Expand Up @@ -275,17 +276,17 @@ public async Task SendRequestBodyAsync(CancellationToken cancellationToken)
sendReset = _responseCompletionState == StreamCompletionState.Failed;
Complete();
}
}

if (sendReset)
{
SendReset();
}
else
{
// Send EndStream asynchronously and without cancellation.
// If this fails, it means that the connection is aborting and we will be reset.
_connection.LogExceptions(_connection.SendEndStreamAsync(StreamId));
if (sendReset)
{
SendReset();
}
else
{
// Send EndStream asynchronously and without cancellation.
// If this fails, it means that the connection is aborting and we will be reset.
_connection.LogExceptions(_connection.SendEndStreamAsync(StreamId));
}
}
}
}
Expand Down Expand Up @@ -325,7 +326,7 @@ public async ValueTask<bool> WaitFor100ContinueAsync(CancellationToken cancellat

private void SendReset()
{
Debug.Assert(!Monitor.IsEntered(SyncObject));
Debug.Assert(Monitor.IsEntered(SyncObject));
Debug.Assert(_requestCompletionState != StreamCompletionState.InProgress);
Debug.Assert(_responseCompletionState != StreamCompletionState.InProgress);
Debug.Assert(_requestCompletionState == StreamCompletionState.Failed || _responseCompletionState == StreamCompletionState.Failed,
Expand All @@ -336,6 +337,8 @@ private void SendReset()
// Don't send a RST_STREAM if we've already received one from the server.
if (_resetException == null)
{
// If execution reached this line, it's guaranteed that
// _requestCompletionState == StreamCompletionState.Failed or _responseCompletionState == StreamCompletionState.Failed
_connection.LogExceptions(_connection.SendRstStreamAsync(StreamId, Http2ProtocolErrorCode.Cancel));
}
}
Expand Down Expand Up @@ -384,9 +387,12 @@ private void Cancel()
// When cancellation propagates, SendRequestBodyAsync will set _requestCompletionState to Failed
requestBodyCancellationSource?.Cancel();

if (sendReset)
lock (SyncObject)
{
SendReset();
if (sendReset)
{
SendReset();
}
}

if (signalWaiter)
Expand Down

0 comments on commit 5d29560

Please sign in to comment.