fix(client-h2): stop double-decrementing kOpenStreams on stream timeout#5076
fix(client-h2): stop double-decrementing kOpenStreams on stream timeout#5076SAY-5 wants to merge 1 commit intonodejs:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5076 +/- ##
==========================================
- Coverage 93.10% 93.10% -0.01%
==========================================
Files 110 110
Lines 35805 35799 -6
==========================================
- Hits 33337 33331 -6
Misses 2468 2468 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mcollina
left a comment
There was a problem hiding this comment.
Thanks for opening a PR! Can you please add a unit test?
Both the 'timeout' handler and the 'close' handler on an h2 stream decremented session[kOpenStreams] and called session.unref when the counter hit zero. Because the 'timeout' path is always followed by the stream actually closing, every timed-out stream ran the decrement twice, leaving the counter one lower than the real number of open streams. On a session that sees N timed-out streams the counter drifts to -N, and the 'close' handler's 'if (session[kOpenStreams] === 0) session.unref()' check never fires again for the lifetime of the session. In single-request scenarios this manifests as the socket never being re-unref'd after a timeout even though no streams remain; in multi-stream scenarios it also causes session.unref to fire while a concurrent stream is still active, because the decrement runs before the close of the timed-out stream. Remove the duplicate decrement from the 'timeout' handler and rely on the unified bookkeeping in 'close', which is guaranteed to run for every stream regardless of how it ended. Add a regression test that drives three sequential bodyTimeout aborts on the same h2 session and asserts the open-streams counter returns to 0. On the unpatched code the counter settles at -3. Closes nodejs#5073 Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
60ba16b to
12f1734
Compare
|
Thanks @mcollina — added a regression test in test/http2-timeout.js. The test drives three sequential bodyTimeout aborts on the same h2 session, then reads the module-local open-streams counter off the session via its Symbol description. With the fix applied the counter returns to 0 after each stream closes; on master (pre-patch) it settles at -3 after the three cycles because the 'timeout' handler and the 'close' handler were both decrementing it. Verified locally on macOS, Node 25.9:
|
Fixes #5073.
The HTTP/2 stream
timeouthandler inlib/dispatcher/client-h2.jsdoes:abort(err)ultimately destroys the http2 stream, which firesclose, and thecloselistener a few lines above already decrementskOpenStreams:Result: a single timed-out request knocks the counter down twice and it ends at
-1instead of0. Future stream accounting on the session is then permanently skewed — the=== 0unref check can never match, and reused-session flows that rely on the counter reaching0to release the socket start holding sessions open longer than they should.Leave the decrement to the
closehandler. Thetimeouthandler only needs to stop the data listener and callabort(); destruction viaabort()guaranteesclosefires exactly once. After this change the repro in the issue reportsopen streams after timeout: 0as expected.