fix(h2): destroy the stream on abort instead of relying on close()#5462
Conversation
| stream.close() | ||
| if (!stream.destroyed) { | ||
| util.destroy(stream) | ||
| } |
There was a problem hiding this comment.
I think we should delay this by at least a setImmediate, allowing for the rst to fire.
There was a problem hiding this comment.
Good call. Updated so the destroy is deferred with a setImmediate, giving the RST_STREAM frame queued by close() a chance to be written first:
stream.close()
setImmediate(() => {
if (!stream.destroyed) {
util.destroy(stream)
}
})Also rebased on latest main to resolve the conflicts from the h2 hot-path refactor (#5483), and adjusted the test to await the deferred destroy instead of asserting synchronously.
metcoder95
left a comment
There was a problem hiding this comment.
LGTM with @mcollina comment
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5462 +/- ##
==========================================
- Coverage 93.43% 93.43% -0.01%
==========================================
Files 110 110
Lines 37278 37291 +13
==========================================
+ Hits 34829 34841 +12
- Misses 2449 2450 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
8da76e6 to
205a156
Compare
What
On abort, the HTTP/2 client sends
RST_STREAMviastream.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 nativeHttp2Streambe released). This adds an explicitstream.destroy()afterclose()so the handle is released deterministically.Why
If that
'close'event never fires — which can happen for an aborted stream on a busy, long-lived multiplexed session — the nativeHttp2Streamstays alive (pinned by a V8 global handle viaowner_symbol), and with it the stream's[kRequestStreamState]and the entire request object graph it references (theRequest, itsAbortController, 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
ClientHttp2Streamobjects, 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(anAbortSignal.timeoutfiring 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 aborttotest/http2-dispatcher.js: it captures the in-flight request's stream, aborts, and assertsstream.destroyed === true. It fails on the currentclose()-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 aclose()callback/timeout instead) if maintainers prefer.