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');