Skip to content

Drain HTTP/3 response after trailers #116319

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

antonfirsov
Copy link
Member

Fixes #60118.

Merges the sync and async implementation of Http3RequestStream disposal, since QuicStream.Dispose() does sync over async anyways.

The test changes are built on top of #116113 which is a rather independent and safe change, it would be desireable to merge that PR first.

Note: I wasn't able to reproduce #60118 with the POST Duplex Slow stress case. The server-side PEER_RECEIVE_ABORTED events seen there were results of cancellation and by setting cancelRate=0 they stopped to occur. GetAsync_TrailersWithoutServerStreamClosure_Success reproduces the issue without the product code change.

Copy link
Contributor

@Copilot Copilot AI left a 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 unifies sync/async disposal in Http3RequestStream, ensures HTTP/3 responses are drained after trailers, and refactors test helper stream usage.

  • Merge sync and async Dispose implementations into a single async-driven flow.
  • Introduce DrainResponseAsync to consume leftover response data after trailers.
  • Update Http3LoopbackStream in tests to use a Stream property instead of a private field.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
Http3RequestStream.cs Unified Dispose logic, added response-drain task and helper.
Http3LoopbackStream.cs Replaced _stream field with Stream property and updated refs.
Comments suppressed due to low confidence (2)

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs:581

  • There’s a TODO placeholder here; please add a descriptive comment explaining why it's safe to stop reading after the trailing headers and what extensions are skipped.
// TODO: Add comments here.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs:1423

  • Add a clear comment describing why initiating response drain on trailing headers is correct and what kinds of frames may be skipped safely.
// TODO: add proper comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HTTP/3] We don't drain H3 stream after trailing headers
1 participant