Skip to content

fix(h2): complete request stream on 'end' instead of waiting for 'close'#5560

Merged
mcollina merged 1 commit into
nodejs:mainfrom
staylor:fix/h2-completion-leak
Jul 16, 2026
Merged

fix(h2): complete request stream on 'end' instead of waiting for 'close'#5560
mcollina merged 1 commit into
nodejs:mainfrom
staylor:fix/h2-completion-leak

Conversation

@staylor

@staylor staylor commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

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':

  • request.onResponseEnd(...) + finalizeRequest(...)
  • releaseRequestStream(...) (detaches the request, removes stream listeners)
  • closeStreamSession(...) (decrements the session's open-stream count)
  • stream[kRequestStreamState] = null

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.

  • On main: fails — onResponseEnd is never called (onCompleteCalls: 0) and listeners stay attached.
  • With this change: passes.

All 24 test/http2-*.js files pass (including Dispatcher#Connect, trailers, goaway, timeout, and abort suites); eslint is clean.

Refs #5558.

@staylor
staylor force-pushed the fix/h2-completion-leak branch from a6eefca to aa443d5 Compare July 14, 2026 22:38
Comment thread lib/dispatcher/client-h2.js Outdated
// 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.

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.

shorten this comment, or remove

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Cut to two lines — just the idempotency contract; the mechanics are clear from the code.

Comment thread lib/dispatcher/client-h2.js Outdated
// 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.

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.

This comment is too long. It doesn't explain why this is needed but reference an issue. Update.

This should mention "blocked event loop".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@staylor
staylor force-pushed the fix/h2-completion-leak branch from aa443d5 to d3f53f9 Compare July 15, 2026 12:59
@codecov-commenter

codecov-commenter commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.45%. Comparing base (6010668) to head (e6a99b4).

Additional details and impacted files
@@           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.
📢 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.

Comment thread lib/dispatcher/client-h2.js
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
@staylor
staylor force-pushed the fix/h2-completion-leak branch from d3f53f9 to e6a99b4 Compare July 15, 2026 18:04

@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 merged commit 87270e4 into nodejs:main Jul 16, 2026
36 checks passed
@staylor

staylor commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

I deployed this using pnpm patch, and 8.7.0 has another memory leak somewhere - will report back when I find out where

pi0x pushed a commit to h3js/srvx that referenced this pull request Jul 17, 2026
@github-actions github-actions Bot mentioned this pull request Jul 20, 2026
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.

4 participants