child_process: emit 'close' after explicit disconnect()

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

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();
+  }));
+}));

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