Drain HTTP/3 response after trailers

[antonfirsov]

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.

[Copilot]

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.

[ManickaP]

The routine for draining looks OK with some suggestion for short-circuiting. But how it's handled in Dispose is not ideal.
Also, we'll need to run the benchmarks from #104035 to make sure this doesn't regress it.

[ManickaP]

So this change will only drain the stream if there are trailers, right?
If the message doesn't have the trailers (ends with DATA frame or is just HEADERS without body and trailers), we still could mistakenly abort the reading side. Or am I overlooking something?

[antonfirsov]

So this change will only drain the stream if there are trailers, right?

Yes. Since #60118 talked about only this specific case, and I didn't consider others. Should we do it? If yes should it happen in this PR?

[ManickaP]

Hmmm, what I'm thinking is that if it means just to just plug-in DrainResponseAsync to few other places then let's do it. On the other hand, if it's more complicated we should probably at least think it through before merging this. Just to make sure we're not doing here something that would have to be undone.

[antonfirsov]

If the message doesn't have the trailers (ends with DATA frame or is just HEADERS without body and trailers), we still could mistakenly abort the reading side

I spent more time examining Http3RequestStream code, and I don't think this could happen. If the message ends with a DATA frame (or an uknown frame we skipped), Http3ReadStream.ReadAsync will return with the data read into its' buffer. There will be a subsequent read issued against the Http3ReadStream which will make ReadNextDataFrameAsync hit an EOS and stop processing data by setting _responseDataPayloadRemaining = -1 in case null (and then return 0 from Http3ReadStream.Read(Async)):

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs

Lines 1374 to 1379 in 0d628da

	case null:
	// End of stream.
	CopyTrailersToResponseMessage(response);
	_responseDataPayloadRemaining = -1; // Set to -1 to indicate EOS.
	return false;

The primary issue with the trailer handling logic on main is that we execute the code in case null without actually reading the EOS.

This will short circuit subsequent calls to Http3ReadStream.ReadAsync to return 0 while in fact never reading the EOS from the underlying QuicStream:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs

Lines 1340 to 1344 in 0d628da

	if (_responseDataPayloadRemaining == -1)
	{
	// EOS -- this branch will only be taken if user calls Read again after EOS.
	return false;
	}

[ManickaP]

You could short-circuit this with checking ReadsClosed. Also I'd prefer this to finish synchronously in the most common scenario.

[antonfirsov]

ReadsClosed.IsCompleted is never true at the moment we finish reading the trailers. See my #116319 (comment) above.

[ManickaP]

I'm wondering if we could hit the issue with GC eating the H3 stream while inside WaitForDrainCompletionAndDisposeAsync. For a reference: https://devblogs.microsoft.com/dotnet/keeping-async-methods-alive/

[antonfirsov]

I see us doing the same in HttpContentReadStream (which can get unrooted quickly after returning from an HttpRequest.Dispose):

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs

Line 65 in ff8c934

_ = DrainOnDisposeAsync();

I must admit that I'm confused. According to my understanding of the article, the problem is that the state machine is unrooted (thus all its' references to locals and this), however this doesn't make us worry and run GC.KeepAlive when we fire-and-forget tasks like at the quoted line.

Edit: changed the comment to keep it on point

[antonfirsov]

@stephentoub can you please help and give some pointers what should the right approach here?

[stephentoub]

What is the question?

[antonfirsov]

I'm introducing a method here that is usually called in a fire-and-forget manner:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs

Lines 156 to 163 in bb85d20

	async ValueTask WaitForDrainCompletionAndDisposeAsync()
	{
	Debug.Assert(_responseDrainTask is not null);
	await _responseDrainTask.ConfigureAwait(false);
	AbortStream();
	await _stream.DisposeAsync().ConfigureAwait(false);
	_recvBuffer.Dispose();
	}

The owning Http3RequestStream may become unrooted while WaitForDrainCompletionAndDisposeAsync is running. Can this result in the GC killing the objects under Http3RequestStream thus breaking WaitForDrainCompletionAndDisposeAsync? If the answer is yes, why don't we worry about the same problem in the quoted HttpContentReadStream code where we fire-and-forget a draining Task in a very similar manner?

[stephentoub]

As noted in the linked blog post, async method state machine objects are kept alive by the thing they're awaiting. The question then isn't whether Http3RequestStream is rooted, but whether _responseDrainTask will eventually complete and is thus itself rooted (since the only way it could be completed is if something rooted was referencing it in order to complete it).

[antonfirsov]

/azp run runtime-libraries stress-http

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

@ManickaP I still owe the benchmark runs so no need to rush with the review, but I reworked the implementation based on observations I made about Http3RequestStream's behavior, see #116319 (comment).

The new logic will not start _responseDrainTask unless there are bytes (unknown frames) beyond the trailers which is a corner case. In normal cases we just need to make sure to actually read the EOS after the trailers before setting _responseDataPayloadRemaining = -1.

[antonfirsov]

Benchmarks

I did 5-5 consecutive runs main vs PR, see the results here.

Collecting the RPS values, I think the difference is well below the margin of error. (There seems to be quite a high varience.)

	main	pr
	4,160,659	3,935,511
	3,348,607	3,710,616
	4,032,981	4,388,804
	4,272,599	4,160,746
	4,336,889	4,468,676
avg	4,030,347	4,132,871