Skip to content

Commit 00864ce

Browse files
committed
fix(h2): do not unref a session with open streams
1 parent b73952a commit 00864ce

3 files changed

Lines changed: 92 additions & 1 deletion

File tree

‎lib/dispatcher/client-h2.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,8 @@ function resumeH2 (client) {
406406
const session = client[kHTTP2Session]
407407

408408
if (socket?.destroyed === false) {
409-
if (client[kSize] === 0 || client[kMaxConcurrentStreams] === 0) {
409+
// After an upgrade the queue is empty but its stream is still in use, so never unref while a stream is open.
410+
if (session[kOpenStreams] === 0 && (client[kSize] === 0 || client[kMaxConcurrentStreams] === 0)) {
410411
unrefH2Session(session)
411412
} else {
412413
refH2Session(session)
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
'use strict'
2+
3+
// Client for test/websocket/h2-keep-process-alive.js. Nothing else here may keep
4+
// the event loop alive: the test checks that the WebSocket alone does.
5+
const { Agent, WebSocket } = require('../..')
6+
7+
const dispatcher = new Agent({
8+
allowH2: true,
9+
connect: { rejectUnauthorized: false }
10+
})
11+
12+
const ws = new WebSocket(`wss://localhost:${process.argv[2]}`, { dispatcher })
13+
14+
ws.onopen = () => process.send('open')
15+
ws.onmessage = ({ data }) => process.send(data)
16+
ws.onclose = ({ code, wasClean }) => process.send({ code, wasClean })
17+
ws.onerror = ({ error }) => { throw error }
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
'use strict'
2+
3+
const { test } = require('node:test')
4+
const { fork } = require('node:child_process')
5+
const { once } = require('node:events')
6+
const { createSecureServer } = require('node:http2')
7+
const { join } = require('node:path')
8+
const { WebSocket: WSWebSocket } = require('ws')
9+
const { key, cert } = require('@metcoder95/https-pem')
10+
const { uid } = require('../../lib/web/websocket/constants')
11+
const { runtimeFeatures } = require('../../lib/util/runtime-features')
12+
13+
const crypto = runtimeFeatures.has('crypto')
14+
? require('node:crypto')
15+
: null
16+
17+
// An open WebSocket over h2 must keep the process alive even though the handshake
18+
// completed its request and left the queue empty. The server sends only after the
19+
// child reports 'open': an unref'd session would let the child exit before that.
20+
async function runClient (t, beforeSend = (stream, send) => send()) {
21+
const server = createSecureServer({ key, cert, settings: { enableConnectProtocol: true } })
22+
t.after(() => server.close())
23+
24+
let serverStream
25+
let serverWs
26+
server.on('stream', (stream, headers) => {
27+
stream.respond({
28+
':status': 200,
29+
'sec-websocket-accept': crypto.hash('sha1', `${headers['sec-websocket-key']}${uid}`, 'base64')
30+
})
31+
32+
serverStream = stream
33+
serverWs = new WSWebSocket(null, null, { autoPong: true })
34+
serverWs.setSocket(stream, Buffer.alloc(0), {
35+
maxPayload: 104857600,
36+
skipUTF8Validation: false
37+
})
38+
})
39+
40+
server.listen(0)
41+
await once(server, 'listening')
42+
43+
const child = fork(join(__dirname, '../fixtures/websocket-h2-client.js'), [String(server.address().port)])
44+
t.after(() => child.kill())
45+
46+
const messages = []
47+
child.on('message', (message) => {
48+
messages.push(message)
49+
50+
if (message === 'open') {
51+
beforeSend(serverStream, () => {
52+
serverWs.send('hello')
53+
serverWs.close(1000)
54+
})
55+
}
56+
})
57+
58+
const [code, signal] = await once(child, 'close')
59+
60+
t.assert.strictEqual(signal, null)
61+
t.assert.strictEqual(code, 0)
62+
t.assert.deepStrictEqual(messages, ['open', 'hello', { code: 1000, wasClean: true }])
63+
}
64+
65+
test('an open WebSocket over H2 keeps the process alive', { skip: crypto == null }, async (t) => {
66+
await runClient(t)
67+
})
68+
69+
// SETTINGS_MAX_CONCURRENT_STREAMS = 0 leaves the open stream alone, so it must not
70+
// unref the session either. The settings callback fires on the child's ACK.
71+
test('an open WebSocket over H2 keeps the process alive after the peer stops allowing new streams', { skip: crypto == null }, async (t) => {
72+
await runClient(t, (stream, send) => stream.session.settings({ maxConcurrentStreams: 0 }, send))
73+
})

0 commit comments

Comments
 (0)