diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 273ddd15414b51..c563ab10827cf4 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1224,24 +1224,30 @@ function closeSession(session, code, error) { session.setTimeout(0); session.removeAllListeners('timeout'); - // Destroy any pending and open streams - if (state.pendingStreams.size > 0 || state.streams.size > 0) { - const cancel = new ERR_HTTP2_STREAM_CANCEL(error); - state.pendingStreams.forEach((stream) => stream.destroy(cancel)); - state.streams.forEach((stream) => stream.destroy(error)); - } - // Disassociate from the socket and server. const socket = session[kSocket]; const handle = session[kHandle]; - // Destroy the handle if it exists at this point. + // Destroy the handle *before* destroying streams. When the socket is already + // closed the handle's ondone callback (finishSessionClose) is invoked + // synchronously from handle.destroy(), which schedules the session 'close' + // event via process.nextTick. Destroying the streams afterwards ensures their + // 'close' events are queued on the nextTick queue *after* the session 'close' + // event, so user code receives the session-level signal before any stream + // callbacks observe session.closed/session.destroyed as true. if (handle !== undefined) { handle.ondone = finishSessionClose.bind(null, session, error); handle.destroy(code, socket.destroyed); } else { finishSessionClose(session, error); } + + // Destroy any pending and open streams + if (state.pendingStreams.size > 0 || state.streams.size > 0) { + const cancel = new ERR_HTTP2_STREAM_CANCEL(error); + state.pendingStreams.forEach((stream) => stream.destroy(cancel)); + state.streams.forEach((stream) => stream.destroy(error)); + } } // Upon creation, the Http2Session takes ownership of the socket. The session 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..e23a160246fe17 --- /dev/null +++ b/test/parallel/test-http2-session-close-before-stream-close.js @@ -0,0 +1,68 @@ +'use strict'; + +// Regression test: when the server abruptly destroys the underlying socket, +// the client session 'close' event must fire before any stream 'close' +// callback can observe session.closed/session.destroyed as true. +// See https://github.com/nodejs/node/issues/ + +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}`); + + let sessionCloseFired = false; + + session.on('close', common.mustCall(() => { + sessionCloseFired = true; + server.close(); + })); + + // We accept that an error may or may not fire, but it must fire before + // any stream close callback sees session.destroyed. + session.on('error', () => {}); + + const req = session.request({ ':path': '/close' }); + req.resume(); + req.on('response', () => {}); + + // The stream 'close' event must not observe the session as destroyed + // before the session 'close' event has fired. + req.on('close', common.mustCall(() => { + // If the session is already destroyed, the session 'close' event must + // have already been emitted (sessionCloseFired === true). + if (session.destroyed) { + assert.strictEqual( + sessionCloseFired, + true, + 'session "close" event must fire before stream "close" callback ' + + 'observes session.destroyed === true', + ); + } + })); +})); 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');