quic: do not destroy incoming streams that have a consumer - #65335
Open
trivenay wants to merge 2 commits into
Open
quic: do not destroy incoming streams that have a consumer#65335trivenay wants to merge 2 commits into
trivenay wants to merge 2 commits into
Conversation
Collaborator
|
Review requested:
|
trivenay
force-pushed
the
quic-stream-consumer-gate
branch
from
August 16, 2026 19:49
3b6d1fb to
aa4680a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65335 +/- ##
==========================================
- Coverage 90.13% 90.11% -0.03%
==========================================
Files 752 752
Lines 251820 251858 +38
Branches 47352 47342 -10
==========================================
- Hits 226974 226953 -21
- Misses 16158 16233 +75
+ Partials 8688 8672 -16
🚀 New features to boost your workflow:
|
jasnell
reviewed
Aug 17, 2026
jasnell
approved these changes
Aug 17, 2026
An incoming stream was destroyed unless the session had an onstream callback, even when session-level stream callbacks (onheaders et al) were registered and the negotiated application (HTTP/3) would drive the stream through them. Users had to register stub onstream handlers just to keep their streams alive. Destroy an incoming stream only when the session has no consumer for it at all: no onstream callback, and no session-level stream callbacks runnable on the negotiated application (checked via the existing headersSupported session state, computed when the application is selected from ALPN). Sessions with no consumers keep the current destroy-and-warn behavior so unconsumed streams cannot accumulate and hold flow control credit. On HTTP/3 sessions only bidirectional request streams reach this path; control and QPACK streams are consumed internally by nghttp3 and are never exposed to JavaScript. Fixes: nodejs#64192 Signed-off-by: Naman Trivedi <trivenay@amazon.com>
Instead of borrowing headersSupported as a proxy, record whether the selected application dispatches the session-level stream callbacks in its own session state field, set in SetApplication from a SupportsStreamCallbacks() capability on the application (mirroring SupportsHeaders()). Today its value coincides with the application being HTTP/3, but it keeps the incoming stream consumer check decoupled from application identity. Signed-off-by: Naman Trivedi <trivenay@amazon.com>
trivenay
force-pushed
the
quic-stream-consumer-gate
branch
from
August 17, 2026 12:45
aa4680a to
ae555c6
Compare
Collaborator
trivikr
reviewed
Aug 17, 2026
trivikr
left a comment
Member
There was a problem hiding this comment.
Can we update quic.md to reflect this behavior? It currently says all peer-initiated streams arrive through session.onstream. This PR makes onstream optional when the negotiated application supports configured stream callbacks - for example, HTTP/3 requests consumed through onheaders. The general stream description, session.onstream docs, and HTTP/3 server example should clarify this.
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
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.
Fixes: #64192
An incoming QUIC stream is currently destroyed at arrival unless the session has an
onstreamcallback. That check is narrower than the ways a stream can actually be consumed: on an HTTP/3 session, the application layer drives incoming request streams through the session-level stream callbacks (onheaders,ontrailers,oninfo,onwanttrailers), andonstreamis legitimately absent. Today such streams are destroyed out from under the h3 machinery, and the practical workaround is registering stubonstreamhandlers just to keep streams alive (as noted in the issue and in the WebTransport work in #63827).This changes the arrival check to the broader invariant discussed in the issue: destroy an incoming stream only when the session has no consumer at all for it.
A consumer is either:
onstreamcallback (any negotiated protocol), orheadersSupportedsession state, which the C++ side computes once when it selects the application from the negotiated ALPN — so the JS side reads a stored conclusion rather than re-deriving ALPN rules.Notes on scope and safety:
MAX_STREAMSslots. (Kept streams that go idle are additionally bounded bystreamIdleTimeout.)onsessionid(quic: Webtransport support quic/http 3 #63827) lands in the session-level stream callbacks, it participates with no further changes.Http3Application::ReceiveStreamOpen), so only bidirectional request streams ever reach this check.listen()/connect()options), attached synchronously before the handshake that must complete before any peer stream can arrive, and the decision reads only session configuration — never stream content.The new test covers both sides of the predicate: an HTTP/3 request completes on a session with only
onheadersregistered and noonstream(destroyed before this change), and stream callbacks registered on a session whose negotiated application cannot run them do not keep streams alive (destroyed with the warning, as before). The test was also validated against a build without this change, where the first case fails as expected. The existingtest-quic-stream-uni-no-onstreamcontinues to cover the no-callbacks-at-all case.