quic: do not destroy incoming streams that have a consumer - #65335
Open
trivenay wants to merge 1 commit into
Open
quic: do not destroy incoming streams that have a consumer#65335trivenay wants to merge 1 commit into
trivenay wants to merge 1 commit into
Conversation
Collaborator
|
Review requested:
|
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>
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.11% 90.12% +0.01%
==========================================
Files 752 752
Lines 251569 251592 +23
Branches 47268 47272 +4
==========================================
+ Hits 226701 226756 +55
+ Misses 16233 16178 -55
- Partials 8635 8658 +23
🚀 New features to boost your workflow:
|
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.