Remove Http3RequestStream from _activeRequest when response is read/aborted. #72670
Conversation
ghost
added
the
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #58072. This PR makes sure the Http3RequestStream is removed from _activeRequests immediately after the request is read completely or aborted. This allows closing the Http3Connection before all the response streams are disposed.
|
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Outdated
Show resolved
Hide resolved
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Do we have any test coverage for the scenario Geoff described in the orig issue? He mentions |
The test Geoff mentioned is running for Http3, I removed the using which was put there because of HTTP3 so the scenario is exercised. However, the only difference with this PR is that the test does not run for 1m but for 300ms. Do you think we should add a time limit (say, 30s)? |
Sounds good. And if it causes any troubles we can just remove it. |
…ream-is-not-removed-from-_activeRequests-table-until-response-Stream-Dispose-is-called
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Test failure is #72756 |
|
|
||
| private enum StreamCompletionState : byte | ||
| { | ||
| InProgress, | ||
| Completed, | ||
| Failed | ||
| } |
| } | ||
|
|
||
| _requestSendCompleted = true; | ||
| RemoveFromConnectionIfDone(); |
There was a problem hiding this comment.
What happens in exceptional cases? Is it handled somehow differently? Same question for _responseRecvCompleted.
There was a problem hiding this comment.
_responseRecvCompleted is set to true in HandleReadResponseContentException so it is set in exceptional cases too.
I moved _requestSendCompleted to a finally block to make sure it covers both successful and unsuccessful paths.
| private bool _requestSendCompleted; | ||
| private bool _responseRecvCompleted; |
There was a problem hiding this comment.
How does this differ from _responseDataPayloadRemaining and _requestContentLengthRemaining?
Just making sure we need these fields and cannot use something we already have.
There was a problem hiding this comment.
_responseDataPayloadRemaining is only for the current DATA frame. _requestContentLengthRemaining seems to be usable.
There was a problem hiding this comment.
Unfortunately, I think we need both actually because with plain _requestContentLengthRemaining I can't distinguish failed request sends.
3 participants
Fixes #58072.
This PR makes sure the Http3RequestStream is removed from _activeRequests immediately after the request is read completely or aborted. This allows closing the Http3Connection before all the response streams are disposed.