Refactor HTTP/3 server stream handling to consolidate exception handling for stream resets #122561
+47
−52
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add try-catch block around ReadAsync in ProcessServerStreamAsync to handle QuicException with StreamAborted error when reading multi-byte stream type headers with tracing enabled. This ensures compliance with RFC 9114 which requires tolerating stream resets prior to receiving the complete unidirectional stream header. Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix handling of unidirectional server-initiated streams in Http3Connection
Fix unhandled QuicException when server resets unidirectional stream during tracing
Dec 15, 2025
MihaZupan
approved these changes
Dec 15, 2025
Member
MihaZupan
left a comment
MihaZupan
left a comment
There was a problem hiding this comment.
I'd also be fine with deleting this whole if (NetEventSource.Log.IsEnabled()) block if we don't see the value in this particular message.
rzikm
reviewed
Dec 16, 2025
Comment on lines
671
to
692
| while (!VariableLengthIntegerHelper.TryRead(buffer.ActiveSpan, out unknownStreamType, out _)) | ||
| { | ||
| buffer.EnsureAvailableSpace(VariableLengthIntegerHelper.MaximumEncodedLength); | ||
| bytesRead = await stream.ReadAsync(buffer.AvailableMemory, CancellationToken.None).ConfigureAwait(false); | ||
|
|
||
| try | ||
| { | ||
| bytesRead = await stream.ReadAsync(buffer.AvailableMemory, CancellationToken.None).ConfigureAwait(false); | ||
| } | ||
| catch (QuicException ex) when (ex.QuicError == QuicError.StreamAborted) | ||
| { | ||
| // Treat identical to receiving 0. Stream was reset before we could read the full stream type header. | ||
| bytesRead = 0; | ||
| } | ||
|
|
||
| if (bytesRead == 0) | ||
| { | ||
| unknownStreamType = -1; | ||
| break; | ||
| } | ||
|
|
||
| buffer.Commit(bytesRead); | ||
| } |
Member
There was a problem hiding this comment.
I would prefer doing this before the switch, where we perform the first read on the stream, so that we have only one place that we need to make resistant to early stream resets. For common cases, we are still going to do only a single iteration.
Member
There was a problem hiding this comment.
I'm wondering if we could try to craft test for it as well. If I understand it correctly all we need is to send reset before the client tries to read, right?
Member
There was a problem hiding this comment.
If I understand it correctly all we need is to send reset before the client tries to read, right?
This will verify the initial read case (existing try-catch on the first read), but we also need to cover the subsequent reads case (the one being fixed here).
It's a bit more complicated but doable: the stream type needs to be a multi-byte varint, and the stream needs to be reset after the client has read the first byte(s), but before it had a chance to get the rest of them.
Member
There was a problem hiding this comment.
I would prefer doing this before the switch, where we perform the first read on the stream, so that we have only one place that we need to make resistant to early stream resets.
I agree. And I think we can as well read and log the stream type for bidirectional streams. BTW it will be beneficial for the future WebTransport implementation, which allows the server to initiate both uni- and bidirectional streams with new stream types.
Contributor
Author
There was a problem hiding this comment.
Refactored in a7877d9. The stream type reading is now done once before the switch statement with proper exception handling, and all server-initiated streams (including bidirectional) are logged when tracing is enabled.
Co-authored-by: CarnaViire <3184057+CarnaViire@users.noreply.github.com>
Co-authored-by: CarnaViire <3184057+CarnaViire@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
Fix unhandled QuicException when server resets unidirectional stream during tracing
Refactor HTTP/3 server stream handling to consolidate exception handling for stream resets
Dec 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When NetEventSource tracing is enabled, the HTTP/3 client throws an unhandled
QuicExceptionif the server resets a unidirectional stream while the client reads a multi-byte stream type header. This violates RFC 9114, which requires tolerating stream resets before receiving the complete stream header.Refactored the stream type reading logic to consolidate exception handling in a single location before the switch statement:
QuicExceptionwithQuicError.StreamAbortedthat covers all reads needed to parse the stream type, not just the diagnostic logging pathThe refactoring consolidates the exception handling pattern that existed at line 598 to cover all stream type reads, removing 32 lines of duplicate code while ensuring RFC 9114 compliance.
Customer Impact
With tracing enabled, server-side stream resets (legitimate per RFC 9114) cause the entire HTTP/3 connection to abort with
HttpProtocolException, breaking all in-flight requests. Without this fix, enabling diagnostics paradoxically makes the client less robust.Regression
No. This is a pre-existing bug exposed only when NetEventSource tracing is enabled and the server sends a stream type ≥ 0x40 (requiring 2+ bytes) then resets before sending all bytes.
Testing
Risk
Low. Refactoring moves existing exception handling logic to cover all stream type reads instead of only the first byte. No behavior change when streams complete normally. Actually reduces risk by eliminating code duplication and making the exception handling more comprehensive.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.