From 5ed7523e7ff99445fadecef2cfe842a4b3be0b07 Mon Sep 17 00:00:00 2001 From: Mohamed Sayed Date: Sun, 30 Aug 2026 14:09:17 +0300 Subject: [PATCH] child_process: emit 'close' after explicit disconnect() An explicit subprocess.disconnect() closed the IPC channel without counting it towards the subprocess 'close' event, so 'close' was never emitted once the child exited. Route every channel teardown through the same finish() so the accounting happens where the channel is closed, and drop the handle queue on EOF so the disconnect is not postponed forever. Fixes: https://github.com/nodejs/node/issues/19433 Refs: https://github.com/nodejs/node/pull/19566 Signed-off-by: Mohamed Sayed --- lib/internal/child_process.js | 65 +++++++++++++++---- .../test-child-process-close-handle-queue.js | 63 ++++++++++++++++++ .../test-child-process-disconnect-close.js | 55 ++++++++++++++++ 3 files changed, 170 insertions(+), 13 deletions(-) create mode 100644 test/parallel/test-child-process-close-handle-queue.js create mode 100644 test/parallel/test-child-process-disconnect-close.js diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index a12b2954db81..8b1478780c2b 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -351,6 +351,28 @@ function closePendingHandle(target) { target._pendingMessage = null; } +// Drops the messages queued behind a handle that is still waiting for an +// acknowledgement. Used when the channel is gone for good, so that they get +// the same error as a message sent over a closed channel. +function clearHandleQueue(target) { + const queue = target._handleQueue; + target._handleQueue = null; + + for (let i = 0; i < queue.length; i++) { + const { callback, options } = queue[i]; + + if (options.swallowErrors) + continue; + + const ex = new ERR_IPC_CHANNEL_CLOSED(); + if (typeof callback === 'function') { + process.nextTick(callback, ex); + } else { + process.nextTick(() => target.emit('error', ex)); + } + } +} + ChildProcess.prototype.spawn = function spawn(options) { let i = 0; @@ -662,14 +684,40 @@ function setupChannel(target, channel, serializationMode) { } } else { this.buffering = false; - target.disconnect(); channel.onread = nop; - channel.close(); - target.channel = null; - maybeClose(target); + + // The other side has closed the channel: nothing can be sent or + // received anymore, so do not wait for the acknowledgement of a + // pending handle before disconnecting. + if (target._handleQueue) + clearHandleQueue(target); + + if (target.connected) { + target.disconnect(); + } else if (target.channel) { + // disconnect() was called but was postponed by the handle queue. + target._disconnect(); + } else { + // disconnect() was called but was postponed until the message being + // read completes, which is not going to happen anymore. + finish(); + } } }; + // Closes the channel for good and does the bookkeeping that depends on it: + // emit 'disconnect' and count the channel towards the 'close' event of the + // subprocess. Every way of tearing down the channel ends up here. + let finished = false; + function finish() { + if (finished) return; + finished = true; + + channel.close(); + target.emit('disconnect'); + maybeClose(target); + } + // Object where socket lists will live channel.sockets = { got: {}, send: {} }; @@ -948,15 +996,6 @@ function setupChannel(target, channel, serializationMode) { if (this._pendingMessage) closePendingHandle(this); - let fired = false; - function finish() { - if (fired) return; - fired = true; - - channel.close(); - target.emit('disconnect'); - } - // If a message is being read, then wait for it to complete. if (channel.buffering) { this.once('message', finish); diff --git a/test/parallel/test-child-process-close-handle-queue.js b/test/parallel/test-child-process-close-handle-queue.js new file mode 100644 index 000000000000..099307efaf1c --- /dev/null +++ b/test/parallel/test-child-process-close-handle-queue.js @@ -0,0 +1,63 @@ +'use strict'; +// Tests that when the IPC channel is closed by the other side while messages +// are still queued behind a handle waiting for its acknowledgement, the queue +// is dropped (callbacks are called with ERR_IPC_CHANNEL_CLOSED, or 'error' is +// emitted for messages without a callback) and 'close' is still emitted. +const common = require('../common'); +const assert = require('assert'); +const net = require('net'); +const { fork } = require('child_process'); +const fixtures = require('../common/fixtures'); + +const server = net.createServer().listen(0, common.mustCall(() => { + // The child exits without ever reading from the channel, so the handle is + // never acknowledged and everything sent after it stays queued. + const child = fork(fixtures.path('exit.js'), ['7']); + + let gotExit = false; + let gotClose = false; + + // The handle itself is written right away. + child.send('handle', server, common.mustCall((err) => { + assert.strictEqual(err, null); + })); + + // Queued behind the handle: with a callback... + assert.strictEqual( + child.send('queued', common.mustCall((err) => { + assert.strictEqual(err.code, 'ERR_IPC_CHANNEL_CLOSED'); + assert.strictEqual(gotClose, false); + })), + true, + ); + // ... without a callback ... + assert.strictEqual(child.send('queued'), false); + // ... and with errors swallowed. + assert.strictEqual(child.send('queued', undefined, { swallowErrors: true }), false); + + child.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ERR_IPC_CHANNEL_CLOSED'); + assert.strictEqual(gotClose, false); + })); + + child.on('disconnect', common.mustCall(() => { + assert.strictEqual(child._handleQueue, null); + assert.strictEqual(child._pendingMessage, null); + })); + + child.on('exit', common.mustCall((code, signal) => { + gotExit = true; + assert.strictEqual(code, 7); + assert.strictEqual(signal, null); + })); + + child.on('close', common.mustCall((code, signal) => { + gotClose = true; + assert.strictEqual(gotExit, true); + assert.strictEqual(code, 7); + assert.strictEqual(signal, null); + assert.strictEqual(child.connected, false); + assert.strictEqual(child.channel, null); + server.close(); + })); +})); diff --git a/test/parallel/test-child-process-disconnect-close.js b/test/parallel/test-child-process-disconnect-close.js new file mode 100644 index 000000000000..83c3e01d8261 --- /dev/null +++ b/test/parallel/test-child-process-disconnect-close.js @@ -0,0 +1,55 @@ +'use strict'; +// Regression test for https://github.com/nodejs/node/issues/19433: +// after the parent explicitly calls subprocess.disconnect(), the subprocess +// must still emit 'close' (exactly once) when it exits and its stdio closes, +// regardless of whether it exits on its own or is killed. +const common = require('../common'); +const assert = require('assert'); +const { fork } = require('child_process'); + +if (process.argv[2] === 'child') { + const mode = process.argv[3]; + // Keep the event loop alive until the parent disconnects (or kills us). + const timer = setInterval(() => {}, 1000); + process.on('disconnect', () => { + clearInterval(timer); + if (mode === 'self-exit') + process.exit(42); + }); + process.send('ready'); + return; +} + +function test(mode, expectedCode, expectedSignal) { + const child = fork(__filename, ['child', mode]); + const events = []; + + child.on('disconnect', common.mustCall(() => { + events.push('disconnect'); + assert.strictEqual(child.connected, false); + assert.strictEqual(child.channel, null); + })); + + child.on('exit', common.mustCall((code, signal) => { + events.push('exit'); + assert.strictEqual(code, expectedCode); + assert.strictEqual(signal, expectedSignal); + })); + + child.on('close', common.mustCall((code, signal) => { + events.push('close'); + assert.strictEqual(code, expectedCode); + assert.strictEqual(signal, expectedSignal); + assert.deepStrictEqual(events, ['disconnect', 'exit', 'close']); + })); + + child.once('message', common.mustCall((message) => { + assert.strictEqual(message, 'ready'); + child.disconnect(); + if (mode === 'kill') + child.kill('SIGKILL'); + })); +} + +test('self-exit', 42, null); +test('kill', null, 'SIGKILL');