Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/dispatcher/client-h2.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should delay this by at least a setImmediate, allowing for the rst to fire.


// We move the running index to the next request
client[kOnError](err)
Expand Down
74 changes: 74 additions & 0 deletions test/http2-dispatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 })

Expand Down