fix(h2): complete request stream on 'end' instead of waiting for 'close' - #5560
fix(h2): complete request stream on 'end' instead of waiting for 'close'#5560mcollina merged 1 commit into
Conversation
Sorry, something went wrong.
| // fires first wins, and the null-state guard makes the later call a no-op. This | ||
| // lets a completed stream be released without waiting for a 'close' event that | ||
| // can fail to fire on a busy, long-lived multiplexed session (#5558), which | ||
| // otherwise pins the request graph and buffers until OOM. |
There was a problem hiding this comment.
shorten this comment, or remove
Sorry, something went wrong.
There was a problem hiding this comment.
Cut to two lines — just the idempotency contract; the mechanics are clear from the code.
Sorry, something went wrong.
| // long-lived multiplexed session the native 'close' event can fail to | ||
| // fire, stranding the completed stream's request graph and buffers until | ||
| // OOM (#5558). Completing here releases them deterministically; if 'close' | ||
| // arrives later, completeRequestStream is a no-op. |
There was a problem hiding this comment.
This comment is too long. It doesn't explain why this is needed but reference an issue. Update.
This should mention "blocked event loop".
Sorry, something went wrong.
There was a problem hiding this comment.
Rewritten to lead with the why — a blocked event loop can keep 'close' from firing, stranding the stream's buffers — and dropped the bare issue reference.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #5560 +/- ##
=======================================
Coverage 93.44% 93.45%
=======================================
Files 110 110
Lines 37434 37441 +7
=======================================
+ Hits 34979 34989 +10
+ Misses 2455 2452 -3 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
On a busy, long-lived multiplexed HTTP/2 session the native 'close' event can fail to fire. Since nodejs#5559 the abort path handles this, but normal completion still defers all cleanup (onResponseEnd, listener removal, kRequestStreamState reset, session open-stream decrement) to onRequestStreamClose, gated on 'close'. A completed-but-not-closed stream therefore pins its request graph and buffers for the session's life, leaking until OOM under sustained request volume. Extract that cleanup into completeRequestStream and also run it from onEnd, guarded by a null-state check so whichever of 'end'/'close' fires first wins and the other is a no-op. This completes the response properly (no premature-close of an in-flight body) rather than force-destroying the stream. Refs nodejs#5558. Assisted-By: devx/816ae76b-22d2-4002-873d-05d223e7eec2
mcollina
left a comment
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
|
I deployed this using pnpm patch, and 8.7.0 has another memory leak somewhere - will report back when I find out where |
Sorry, something went wrong.
until nodejs/undici#5560 lands
Problem
On a busy, long-lived multiplexed HTTP/2 session, the native 'close' event can fail to fire for a stream (the same condition #5559 addresses on the abort path). In client-h2.js, normal request completion still defers all cleanup to onRequestStreamClose, which is gated on 'close':
onEnd only sets state.pendingEnd = true and waits. So a stream that completes but never emits 'close' keeps its kRequestStreamState — and through it the request, response body buffers, and stream listeners — alive for the whole session lifetime. Under sustained request volume on a persistent h2 connection this grows without bound until the process OOMs.
We hit this in production on a crawler doing continuous h2 fetches over long-lived sessions: heap-retention profiles showed the request-stream state graph and off-heap response buffers accumulating, with end-of-stream / pipeline / async_hooks teardown machinery retained 15–28× over baseline. #5559 (thank you) fixed the abort path but the completion path kept leaking.
Fix
Extract the terminal cleanup into completeRequestStream(stream) and run it from both onEnd and onRequestStreamClose. A state == null guard makes it idempotent: whichever of 'end' / 'close' fires first performs the cleanup, and the other becomes a no-op (this also prevents a double closeStreamSession decrement).
Completing on 'end' releases the request graph deterministically without waiting for 'close'. Crucially it completes the response properly (onResponseEnd, which ends the consumer body) rather than force-destroying the stream — so an in-flight body is never truncated with ERR_STREAM_PREMATURE_CLOSE. Trailers already stored by onTrailers (which precedes 'end' in the normal flow) are still delivered.
Reproduction / test
Added a test (test/http2-late-data.js) that drives a normal completion through 'response' → 'trailers' → 'data' → 'end' and never emits 'close', then asserts the response completes and the stream listeners are released.
All 24 test/http2-*.js files pass (including Dispatcher#Connect, trailers, goaway, timeout, and abort suites); eslint is clean.
Refs #5558.