-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Http3RequestStream: read EOS after trailers #117026
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug where Http3RequestStream
stopped reading the underlying QUIC stream after processing trailers, causing subsequent reads to immediately report end-of-stream. It refactors trailing-header handling into a new ProcessTrailersAsync
method that issues one more read to detect EOS, and it updates both DrainContentLength0Frames
and ReadNextDataFrameAsync
to use this helper. A new regression test GetAsync_TrailersWithoutServerStreamClosure_Success
is added to cover scenarios where trailers arrive without an explicit server stream closure.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs | Extracted trailer processing into ProcessTrailersAsync and invoked an extra read to consume EOS. |
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs | Added a new theory-based Http3 regression test for reading trailers before stream closure; removed unused usings. |
Tagging subscribers to this area: @dotnet/ncl |
This comment was marked as outdated.
This comment was marked as outdated.
/azp run runtime-libraries stress-http |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This fixes the primary bug captured in #60118 without attempting to drain the stream and without touching the Dispose logic.
We expect trailers to be sent in a single HEADER frame, which is read by
ReadHeadersAsync
but that method doesn't read the EOS. Despite of that, we set_responseDataPayloadRemaining = -1
afterwardsruntime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Lines 577 to 583 in 27c8fe0
which then makes
Http3RequestStream
pretend that it reached EOS in subsequent read calls without ever trying to issue the read to the underlyingQuicStream
:runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Lines 1340 to 1344 in 0d628da
This can be easily fixed by issuing one more read after reading the headers.