Avoid sending EndStream after RST_STREAM with dedicated lock #54552
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsA 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 and actually send EndStream, a concurrent call to Cancel method sends a RST_STREAM frame. 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
|
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
Outdated
Show resolved
Hide resolved
| @@ -384,9 +387,13 @@ private void Cancel() | |||
| // When cancellation propagates, SendRequestBodyAsync will set _requestCompletionState to Failed | |||
| requestBodyCancellationSource?.Cancel(); | |||
|
|
|||
| if (sendReset) | |||
| // SendReset must be called under a lock and after a cancellation of requestBodyCancellationSource. | |||
| lock (SyncObject) | |||
There was a problem hiding this comment.
I'm not really happy to lock on the same object just after leaving the above lock block, but it seems dangerous to change order of requestBodyCancellationSource?.Cancel() and SendReset calls as well as to move requestBodyCancellationSource?.Cancel() under the above lock.
|
@geoffkizer I addressed your comments, please review it again. |
| @@ -384,9 +387,13 @@ private void Cancel() | |||
| // When cancellation propagates, SendRequestBodyAsync will set _requestCompletionState to Failed | |||
| requestBodyCancellationSource?.Cancel(); | |||
|
|
|||
| if (sendReset) | |||
| // SendReset must be called under a lock and after a cancellation of requestBodyCancellationSource. | |||
There was a problem hiding this comment.
Does it actually have to happen after we cancel requestBodyCancellationSource? It seems like the cancellation is going to propagate asynchronously anyway.
There was a problem hiding this comment.
It seems like the cancellation is going to propagate asynchronously anyway.
That was my main concern about reordering. I'm not sure if it always propagates asynchronously. Need take a closer look.
There was a problem hiding this comment.
It seems cancellation callbacks can be invoked on the canceling thread when there is no a synchronization context set, so I believe we shouldn't reorder calls here.
There was a problem hiding this comment.
The cancellation callback will be called on the canceling thread. But that doesn't guarantee anything about when cancellation propagates.
If you don't think we should change the ordering here, that's fine with me; I agree it's safer not to. But let's change or remove the comment because it makes it sound like the ordering is strictly necessary, when I don't think we know that for sure.
There was a problem hiding this comment.
OK. I removed the comment.
ghost
locked as resolved and limited conversation to collaborators
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 and actually send EndStream, a concurrent call to Cancel method sends a RST_STREAM frame. 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