fix(h2): allow stream-bodied requests to multiplex on a busy session#5538
Open
GiHoon1123 wants to merge 2 commits into
Open
fix(h2): allow stream-bodied requests to multiplex on a busy session#5538GiHoon1123 wants to merge 2 commits into
GiHoon1123 wants to merge 2 commits into
Conversation
busy() deferred any request with a stream/async-iterable/FormData body until every other in-flight request on the h2 session completed. For a long-lived stream on the session (e.g. an open text/event-stream GET), this parks bodied requests indefinitely -- the client also stops reporting kBusy while an h2 context is attached, so the pool never opens a second connection either. The two hazards the guard's comments cited are already handled more precisely elsewhere: a stream erroring mid-request now only aborts that one stream (session stays usable, see the abort() closure), and canRetryRequestAfterGoAway() already excludes stream/iterable/FormData bodies from the requests it resurrects after a GOAWAY, erroring them instead of unsafely replaying a partially-consumed body. The blanket pre-dispatch guard was redundant given those. Adds a regression test reproducing the exact deadlock: a POST issued while an SSE GET is held open on the same session now resolves immediately on the shared session instead of hanging. Fixes nodejs#5524.
nodejs#5494) Same busy() fix as the previous commit also resolves nodejs#5494: two stream-bodied POSTs no longer serialize on the same h2 session, the second now dispatches while the first is still in flight instead of waiting for it to complete.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5538 +/- ##
==========================================
- Coverage 93.44% 93.44% -0.01%
==========================================
Files 110 110
Lines 37329 37319 -10
==========================================
- Hits 34883 34871 -12
- Misses 2446 2448 +2 ☔ View full report in Codecov by Harness. 🚀 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.
This relates to...
Fixes #5524
Fixes #5494
Same root cause for both -- #5494 already has an open fix in #5497 (same core change to
busy(), currently blocked on a requested unit test) -- see "Relationship to #5497" below before reviewing.Rationale
busy()inlib/dispatcher/client-h2.jsdeferred any request with a stream/async-iterable/FormData body until every other in-flight request on the h2 session completed. If one of those in-flight requests is a long-lived stream (e.g. an opentext/event-streamGET), every bodied request queued behind it parks indefinitely. The client also stops reportingkBusywhile an h2 context is attached (so the pool keeps multiplexing instead of opening a second connection), so there's no escape route -- for an SSE stream, the hang is permanent.The two hazards the guard's comments cited are already handled more precisely elsewhere in this file:
abort()closure (explicit comment there: "We do not destroy the socket as we can continue using the session"); it doesn't touch sibling streams.canRetryRequestAfterGoAway()already excludes stream/iterable/FormData bodies from the requests it resurrects after a GOAWAY (body == null || isBuffer || isBlobLikeonly), erroring the rest instead of replaying a partially-consumed body.Both of those were already in place before this change, so the blanket pre-dispatch guard was redundant.
Relationship to #5497
#5497 removes the same 9 lines for the same reason, opened against #5494 (concurrent POSTs on h2 not running concurrently -- a throughput complaint). #5524 is a more severe symptom of the identical guard: not just serialized, but a permanent deadlock when one of the concurrent requests is a long-lived stream like SSE. I only found #5497 after finishing this fix independently.
This PR adds what #5497 doesn't currently have:
mcollinarequested on feat(h2): enable concurrency on idempotent requests #5497 (still outstanding as of this PR) -- covering HTTP2 POST requests overfetch()to the same origin can't run concurrently #5494's own scenario directly: a second stream-bodied POST now dispatches while the first is still in flight instead of waiting for it to complete.fetchPOST hangs while an SSE stream is open #5524 deadlock scenario (SSE held open + bodied POST hangs forever, not just serializes).Happy to close this in favor of #5497 if the maintainers would rather continue there -- opening it mainly so the missing test and both scenarios' coverage aren't lost. No preference on which one lands.
Changes
Features
N/A
Bug Fixes
fetchPOST hangs while an SSE stream is open #5524: a bodiedfetch()/request()call on an h2 session no longer waits forever behind an open long-lived stream (e.g. SSE) on the same session.fetch()to the same origin can't run concurrently #5494: concurrent bodiedfetch()/request()calls on the same h2 session now dispatch concurrently instead of serializing.Breaking Changes and Deprecations
N/A -- this only removes a guard that prevented dispatch; nothing that previously worked changes behavior, only what could not previously proceed now can.
Status