From 8da76e6054638b91ad8ea9e2bab77ab5463cfb94 Mon Sep 17 00:00:00 2001 From: Scott Taylor Date: Mon, 29 Jun 2026 10:30:03 -0400 Subject: [PATCH] fix(h2): destroy the stream on abort instead of relying on close() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- lib/dispatcher/client-h2.js | 9 ++++- test/http2-dispatcher.js | 74 +++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/lib/dispatcher/client-h2.js b/lib/dispatcher/client-h2.js index 9f27854385c..0547dc518ae 100644 --- a/lib/dispatcher/client-h2.js +++ b/lib/dispatcher/client-h2.js @@ -834,8 +834,15 @@ function writeH2 (client, request) { if (stream != null) { clearRequestStream(request) - // On Abort, we close the stream to send RST_STREAM frame + // On Abort, we close the stream to send RST_STREAM frame, then destroy it. + // close() alone leaves cleanup waiting on the 'close' event; on a busy, + // long-lived multiplexed session that event can fail to fire, leaving the + // native Http2Stream (and the whole request graph it pins) alive for the + // session's life. destroy() releases the handle deterministically. stream.close() + if (!stream.destroyed) { + util.destroy(stream) + } // We move the running index to the next request client[kOnError](err) diff --git a/test/http2-dispatcher.js b/test/http2-dispatcher.js index e0092595260..6da0c32a538 100644 --- a/test/http2-dispatcher.js +++ b/test/http2-dispatcher.js @@ -772,6 +772,80 @@ test('Should clear h2 request stream references after abort before response', as await t.completed }) +test('Should destroy the h2 stream synchronously on abort', async t => { + t = tspl(t, { plan: 2 }) + + const server = createSecureServer(await pem.generate({ opts: { keySize: 2048 } })) + + // Never respond, so the stream stays in-flight when we abort it. + server.on('stream', (stream) => { + stream.on('error', () => {}) + }) + + after(() => server.close()) + await once(server.listen(0), 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { rejectUnauthorized: false }, + allowH2: true + }) + after(() => client.close()) + + const abortReason = new Error('abort after h2 stream assigned') + let abortRequest = null + + const waitFor = async (fn) => { + for (let i = 0; i < 100; i++) { + const value = fn() + if (value != null) return value + await sleep(10) + } + throw new Error('timed out waiting for h2 request stream') + } + + const responseError = new Promise(resolve => { + client.dispatch({ path: '/', method: 'GET' }, { + onRequestStart (controller) { + abortRequest = controller.abort.bind(controller) + }, + onResponseStart () { + t.fail('unexpected response') + }, + onResponseData () { + return true + }, + onResponseEnd () { + t.fail('unexpected response end') + }, + onResponseError (_controller, err) { + resolve(err) + } + }) + }) + + const requestStreamSymbol = await waitFor(() => { + const request = client[kQueue][client[kRunningIdx]] + if (request == null) return null + return Object.getOwnPropertySymbols(request).find( + (s) => s.description === 'request stream' + ) + }) + const stream = await waitFor( + () => client[kQueue][client[kRunningIdx]]?.[requestStreamSymbol] + ) + + abortRequest = await waitFor(() => abortRequest) + abortRequest(abortReason) + + // The abort path must destroy the stream, not just close() it: relying on the + // async 'close' event leaks the native Http2Stream when that event never + // fires on a busy, long-lived multiplexed session. + t.strictEqual(stream.destroyed, true) + t.strictEqual(await responseError, abortReason) + + await t.completed +}) + test('Should only accept valid ping interval values', async t => { const planner = tspl(t, { plan: 3 })