Skip to content

fix(h2): allow stream-bodied requests to multiplex on a busy session#5538

Open
GiHoon1123 wants to merge 2 commits into
nodejs:mainfrom
GiHoon1123:fix-issue-5524-h2-sse-post-hang
Open

fix(h2): allow stream-bodied requests to multiplex on a busy session#5538
GiHoon1123 wants to merge 2 commits into
nodejs:mainfrom
GiHoon1123:fix-issue-5524-h2-sse-post-hang

Conversation

@GiHoon1123

@GiHoon1123 GiHoon1123 commented Jul 9, 2026

Copy link
Copy Markdown

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() in lib/dispatcher/client-h2.js deferred 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 open text/event-stream GET), every bodied request queued behind it parks indefinitely. The client also stops reporting kBusy while 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:

  • "can error while other requests are inflight and indirectly error those as well" -- a stream erroring mid-request now only aborts that one stream via the abort() closure (explicit comment there: "We do not destroy the socket as we can continue using the session"); it doesn't touch sibling streams.
  • "cannot be retried... could cause failure" -- canRetryRequestAfterGoAway() already excludes stream/iterable/FormData bodies from the requests it resurrects after a GOAWAY (body == null || isBuffer || isBlobLike only), 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:

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

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

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.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina requested a review from metcoder95 July 9, 2026 10:03
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.44%. Comparing base (3c662a5) to head (46eda23).
⚠️ Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTP/2: fetch POST hangs while an SSE stream is open HTTP2 POST requests over fetch() to the same origin can't run concurrently

3 participants