diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 273ddd15414b51..7c6b684324db7c 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1809,11 +1809,24 @@ class ClientHttp2Session extends Http2Session { request(headersParam, options) { debugSessionObj(this, 'initiating request'); - if (this.destroyed) - throw new ERR_HTTP2_INVALID_SESSION(); - - if (this.closed) - throw new ERR_HTTP2_GOAWAY_SESSION(); + // Session-state errors are deferred asynchronously rather than thrown + // synchronously. This prevents a race where stream 'close' callbacks + // observe session.destroyed/closed before the session 'close' event has + // fired: with a sync throw, code that retries inside a stream callback + // would crash before its session lifecycle handlers could clear the cached + // session. By returning a stream that emits 'error' on the next tick we + // match the behaviour of session.request() while connecting, where errors + // are also deferred, and give event-driven code a chance to handle both + // session and stream cleanup in any order. + if (this.destroyed || this.closed) { + // eslint-disable-next-line no-use-before-define + const stream = new ClientHttp2Stream(this, undefined, undefined, {}); + const err = this.destroyed + ? new ERR_HTTP2_INVALID_SESSION() + : new ERR_HTTP2_GOAWAY_SESSION(); + process.nextTick(() => stream.destroy(err)); + return stream; + } this[kUpdateTimer](); diff --git a/test/parallel/test-http2-session-close-before-stream-close.js b/test/parallel/test-http2-session-close-before-stream-close.js new file mode 100644 index 00000000000000..ccbc764122d9f2 --- /dev/null +++ b/test/parallel/test-http2-session-close-before-stream-close.js @@ -0,0 +1,70 @@ +'use strict'; + +// Regression test: when the server abruptly destroys the underlying socket, +// session.request() must not throw synchronously inside a stream 'close' +// callback. Instead it should return a stream that emits 'error' async with +// ERR_HTTP2_INVALID_SESSION, giving session lifecycle handlers a chance to +// clear their cached session reference before any retry occurs. + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const http2 = require('node:http2'); + +const server = http2.createServer(); +let serverSocket; + +server.on('connection', (socket) => { + serverSocket = socket; + socket.on('error', () => {}); +}); + +server.on('sessionError', () => {}); +server.on('stream', (stream, headers) => { + if (headers[':path'] === '/close') { + stream.respond({ ':status': 200 }); + stream.write('partial', () => { + setImmediate(() => serverSocket.destroy()); + }); + return; + } + stream.respond({ ':status': 200 }); + stream.end('ok'); +}); + +server.listen(0, common.mustCall(() => { + const session = http2.connect(`http://localhost:${server.address().port}`); + + session.on('close', common.mustCall(() => server.close())); + session.on('error', () => {}); + + const req = session.request({ ':path': '/close' }); + req.resume(); + req.on('response', () => {}); + req.on('error', () => {}); // socket may emit ECONNRESET on abrupt close + + req.on('close', common.mustCall(() => { + if (!session.destroyed) return; + + // session.request() must NOT throw synchronously even though the session + // is already destroyed. It should return a stream that errors async. + let threw = false; + let req2; + try { + req2 = session.request({ ':path': '/again' }); + } catch { + threw = true; + } + + assert.strictEqual(threw, false, + 'session.request() must not throw synchronously inside a stream close callback'); + + // The returned stream should asynchronously emit ERR_HTTP2_INVALID_SESSION + req2.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ERR_HTTP2_INVALID_SESSION'); + })); + req2.resume(); + })); +})); diff --git a/test/parallel/test-http2-session-close-order-simulation.js b/test/parallel/test-http2-session-close-order-simulation.js new file mode 100644 index 00000000000000..e575cf5c789a85 --- /dev/null +++ b/test/parallel/test-http2-session-close-order-simulation.js @@ -0,0 +1,61 @@ +'use strict'; +// Simulates the nextTick ordering in closeSession to validate the fix logic +// without requiring a compiled binary. + +const assert = require('assert'); + +function simulate(label, handleFirst) { + const events = []; + const ticks = []; + + function nextTick(fn) { ticks.push(fn); } + function drainTicks() { while (ticks.length) ticks.shift()(); } + + // stream.destroy() schedules stream 'close' on nextTick + function destroyStream() { + nextTick(() => events.push('stream close')); + } + + // finishSessionClose (socket already destroyed) schedules session 'close' on nextTick + function finishSessionClose() { + nextTick(() => events.push('session close')); + } + + // handle.destroy() calls ondone synchronously when socket is already destroyed + // and there are no writes in progress (the scenario from the bug report) + function destroyHandle(ondone) { + ondone(); // synchronous, as the C++ Http2Session::Close does + } + + function closeSession() { + if (handleFirst) { + // Our fix: destroy handle first → session 'close' queued first + destroyHandle(finishSessionClose); + destroyStream(); + } else { + // Old code: destroy streams first → stream 'close' queued first + destroyStream(); + destroyHandle(finishSessionClose); + } + } + + closeSession(); + drainTicks(); + return events; +} + +const before = simulate('BEFORE fix', false); +const after = simulate('AFTER fix', true); + +console.log('BEFORE fix ordering:', before.join(' -> ')); +console.log('AFTER fix ordering:', after.join(' -> ')); + +// Old ordering: stream close fires before session close (the bug) +assert.deepStrictEqual(before, ['stream close', 'session close'], + 'Expected old code to have wrong order (stream before session)'); + +// New ordering: session close fires first (the fix) +assert.deepStrictEqual(after, ['session close', 'stream close'], + 'session "close" must fire before stream "close"'); + +console.log('OK');