Skip to content

fix(h2): destroy the stream on abort instead of relying on close()#5462

Open
staylor wants to merge 1 commit into
nodejs:mainfrom
staylor:fix-h2-abort-stream-destroy
Open

fix(h2): destroy the stream on abort instead of relying on close()#5462
staylor wants to merge 1 commit into
nodejs:mainfrom
staylor:fix-h2-abort-stream-destroy

Conversation

@staylor

@staylor staylor commented Jun 29, 2026

Copy link
Copy Markdown

What

On abort, the HTTP/2 client sends RST_STREAM via stream.close() and then leaves the rest of teardown to the stream's asynchronous 'close' event (onRequestStreamClose, which clears the request↔stream references and lets the native Http2Stream be released). This adds an explicit stream.destroy() after close() so the handle is released deterministically.

// lib/dispatcher/client-h2.js — abort()
stream.close()
if (!stream.destroyed) {
  util.destroy(stream)
}

Why

If that 'close' event never fires — which can happen for an aborted stream on a busy, long-lived multiplexed session — the native Http2Stream stays alive (pinned by a V8 global handle via owner_symbol), and with it the stream's [kRequestStreamState] and the entire request object graph it references (the Request, its AbortController, response buffers, and handler closures). On a connection that is intentionally kept open for a long time, one such stream leaks per aborted request, growing the heap without bound.

This was found in production on a service that routes many requests through a small, long-lived shared HTTP/2 connection pool. A heap snapshot from a leaking process showed tens of thousands of retained ClientHttp2Stream objects, each still holding:

  • [kHandle] = Http2Stream (native stream not destroyed) ← (Global handles)
  • [kRequestStreamState] = { request, … } (cleanup never ran, i.e. 'close' never fired)
  • [kRequest] = null (the request had already aborted)

with the abort reason overwhelmingly TimeoutError: The operation was aborted due to timeout (an AbortSignal.timeout firing mid-request). The counts scaled linearly with process uptime.

The abort path's own comment already states the intended behaviour — "the stream gets destroyed and the session remains to create new streams" — but close() alone does not guarantee it. This change makes the code match that intent. The socket/session is left untouched, so it remains available for new streams.

Test

Adds Should destroy the h2 stream synchronously on abort to test/http2-dispatcher.js: it captures the in-flight request's stream, aborts, and asserts stream.destroyed === true. It fails on the current close()-only path (the stream is not destroyed synchronously) and passes with this change. Existing h2 abort tests are unaffected.

Note on reproduction

The behavioural defect (abort leaves the stream un-destroyed, relying on an async 'close' that may not fire) is reproduced directly by the added test. The full production leak only manifests against a real long-lived multiplexed peer where the 'close' event is dropped; I couldn't reduce that to a minimal local server, so the test asserts the deterministic-teardown behaviour rather than the leak itself. Happy to adjust the approach (e.g. destroy on a close() callback/timeout instead) if maintainers prefer.

When an in-flight HTTP/2 request is aborted, the client sends RST_STREAM via
`stream.close()` and otherwise leaves teardown to the stream's async 'close'
event (which runs `onRequestStreamClose` to drop references and release the
native handle). On a busy, long-lived multiplexed session that 'close' event
can fail to fire, leaving the native Http2Stream pinned (a V8 global handle)
along with the entire request object graph it references — an unbounded
per-aborted-request leak on connections that never close. The abort path's own
comment already states the intent ("the stream gets destroyed"); close() alone
doesn't guarantee it.

Destroy the stream after close() so the handle is released deterministically.

Adds a regression test asserting the stream is destroyed synchronously on abort
(fails on the current close()-only path).

Assisted-By: devx/dc4e1318-0193-4805-81ac-82466879ab08
stream.close()
if (!stream.destroyed) {
util.destroy(stream)
}

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.

I think we should delay this by at least a setImmediate, allowing for the rst to fire.

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.

2 participants