Skip to content

Commit 12f1734

Browse files
committed
fix(client-h2): stop double-decrementing kOpenStreams on stream timeout
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 #5073 Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
1 parent c7a2901 commit 12f1734

File tree

2 files changed

+64
-6
lines changed

2 files changed

+64
-6
lines changed

lib/dispatcher/client-h2.js

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -757,12 +757,6 @@ function writeH2 (client, request) {
757757
stream.on('timeout', () => {
758758
const err = new InformationalError(`HTTP/2: "stream timeout after ${requestTimeout}"`)
759759
stream.removeAllListeners('data')
760-
session[kOpenStreams] -= 1
761-
762-
if (session[kOpenStreams] === 0) {
763-
session.unref()
764-
}
765-
766760
abort(err)
767761
})
768762

test/http2-timeout.js

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ const { once } = require('node:events')
99
const pem = require('@metcoder95/https-pem')
1010

1111
const { Client } = require('..')
12+
const { kHTTP2Session } = require('../lib/core/symbols')
1213

1314
test('Should handle http2 stream timeout', async t => {
1415
t = tspl(t, { plan: 1 })
@@ -55,3 +56,66 @@ test('Should handle http2 stream timeout', async t => {
5556

5657
await t.completed
5758
})
59+
60+
// Regression for the double-decrement of kOpenStreams on stream timeout.
61+
// Both the 'timeout' handler and the 'close' handler used to decrement the
62+
// counter, so after a single timeout kOpenStreams became -1. The follow-up
63+
// request then pre-incremented it to 0 and the client treated the session as
64+
// idle, which produced "unref called on already-unrefed session" style
65+
// regressions in downstream code. With the fix only 'close' decrements, so
66+
// the counter stays accurate and a subsequent request on the same client
67+
// completes normally.
68+
test('http2 stream timeout keeps open-stream counter non-negative', async t => {
69+
t = tspl(t, { plan: 5 })
70+
71+
const server = createSecureServer(await pem.generate({ opts: { keySize: 2048 } }))
72+
73+
server.on('stream', (stream) => {
74+
// Respond with headers but hold the body long enough for the client
75+
// bodyTimeout to fire.
76+
stream.respond({ ':status': 200, 'content-type': 'text/plain' })
77+
setTimeout(() => {
78+
try { stream.end('late') } catch {}
79+
}, 500)
80+
})
81+
82+
after(() => server.close())
83+
await once(server.listen(0), 'listening')
84+
85+
const client = new Client(`https://localhost:${server.address().port}`, {
86+
connect: { rejectUnauthorized: false },
87+
allowH2: true,
88+
bodyTimeout: 50,
89+
pipelining: 1
90+
})
91+
after(() => client.close())
92+
93+
// Drive three sequential request timeouts on the same h2 session. Each one
94+
// should leave the open-stream counter at 0, not at a negative value.
95+
for (let i = 0; i < 3; i++) {
96+
const req = await client.request({ path: `/timeout-${i}`, method: 'GET' })
97+
await t.rejects(req.body.text(), {
98+
message: 'HTTP/2: "stream timeout after 50"'
99+
})
100+
// Let the stream's 'close' event run before the next iteration so the
101+
// close handler has had a chance to update the counter.
102+
await new Promise(resolve => setImmediate(resolve))
103+
}
104+
105+
// Locate the open-stream counter on the session by its Symbol description.
106+
// The Symbol itself is module-local to client-h2.js and not exported, so
107+
// this is the only stable way to observe the counter value from a test.
108+
const session = client[kHTTP2Session]
109+
const openStreamsSymbol = Object.getOwnPropertySymbols(session)
110+
.find(sym => sym.description === 'open streams')
111+
112+
t.ok(openStreamsSymbol, 'open-streams symbol must exist on the h2 session')
113+
// Before the fix each timed-out stream ran both the 'timeout' and 'close'
114+
// decrement paths, so after three cycles the counter drifted to -3. With
115+
// only the 'close' handler decrementing, it returns to 0 after each
116+
// request completes.
117+
t.equal(session[openStreamsSymbol], 0,
118+
`open-stream counter should be 0 after all streams close, got ${session[openStreamsSymbol]}`)
119+
120+
await t.completed
121+
})

0 commit comments

Comments
 (0)