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: #19433
Refs: #19566
Signed-off-by: Mohamed Sayed <k@3zrv.com>

Author: Mohamed Sayed

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