child_process: fix spawn and fork abort behavior

Nitzan Uziely · targos · commit 0bcaf9c4d172 · 2021-09-01T23:21:33.000+02:00
Fix AbortSignal in Spawn which doesn't actually abort the process, and fork can emit an AbortError even if the process was already exited. Add documentation For killSignal. Fixes: #37273 PR-URL: #37325 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
diff --git a/doc/api/child_process.md b/doc/api/child_process.md
@@ -351,6 +351,9 @@ controller.abort();
 <!-- YAML
 added: v0.5.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/37325
+    description: killSignal for AbortSignal was added.
   - version: v14.17.0
     pr-url: https://github.com/nodejs/node/pull/36603
     description: AbortSignal support was added.
@@ -383,6 +386,8 @@ changes:
     messages between processes. Possible values are `'json'` and `'advanced'`.
     See [Advanced serialization][] for more details. **Default:** `'json'`.
   * `signal` {AbortSignal} Allows closing the subprocess using an AbortSignal.
+  * `killSignal` {string} The signal value to be used when the spawned
+    process will be killed by the abort signal. **Default:** `'SIGTERM'`.
   * `silent` {boolean} If `true`, stdin, stdout, and stderr of the child will be
     piped to the parent, otherwise they will be inherited from the parent, see
     the `'pipe'` and `'inherit'` options for [`child_process.spawn()`][]'s
@@ -431,6 +436,9 @@ The `signal` option works exactly the same way it does in
 <!-- YAML
 added: v0.1.90
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/37325
+    description: killSignal for AbortSignal was added.
   - version: v14.17.0
     pr-url: https://github.com/nodejs/node/pull/36432
     description: AbortSignal support was added.
@@ -477,6 +485,8 @@ changes:
   * `windowsHide` {boolean} Hide the subprocess console window that would
     normally be created on Windows systems. **Default:** `false`.
   * `signal` {AbortSignal} allows aborting the execFile using an AbortSignal.
+  * `killSignal` {string} The signal value to be used when the spawned
+    process will be killed by the abort signal. **Default:** `'SIGTERM'`.
 
 * Returns: {ChildProcess}
 
diff --git a/lib/child_process.js b/lib/child_process.js
@@ -610,6 +610,18 @@ function normalizeSpawnArguments(file, args, options) {
   };
 }
 
+function abortChildProcess(child, killSignal) {
+  if (!child)
+    return;
+  try {
+    if (child.kill(killSignal)) {
+      child.emit('error', new AbortError());
+    }
+  } catch (err) {
+    child.emit('error', err);
+  }
+}
+
 
 /**
  * Spawns a new process using the given `file`.
@@ -641,21 +653,19 @@ function spawn(file, args, options) {
     const signal = options.signal;
     // Validate signal, if present
     validateAbortSignal(signal, 'options.signal');
-
+    const killSignal = sanitizeKillSignal(options.killSignal);
     // Do nothing and throw if already aborted
     if (signal.aborted) {
       onAbortListener();
     } else {
       signal.addEventListener('abort', onAbortListener, { once: true });
-      child.once('close',
+      child.once('exit',
                  () => signal.removeEventListener('abort', onAbortListener));
     }
 
     function onAbortListener() {
       process.nextTick(() => {
-        child?.kill?.(options.killSignal);
-
-        child.emit('error', new AbortError());
+        abortChildProcess(child, killSignal);
       });
     }
   }
@@ -877,19 +887,18 @@ function spawnWithSignal(file, args, options) {
   }
   const child = spawn(file, args, opts);
 
-  if (options && options.signal) {
+  if (options?.signal) {
+    const killSignal = sanitizeKillSignal(options.killSignal);
+
     function kill() {
-      if (child._handle) {
-        child._handle.kill(options.killSignal || 'SIGTERM');
-        child.emit('error', new AbortError());
-      }
+      abortChildProcess(child, killSignal);
     }
     if (options.signal.aborted) {
       process.nextTick(kill);
     } else {
-      options.signal.addEventListener('abort', kill);
+      options.signal.addEventListener('abort', kill, { once: true });
       const remove = () => options.signal.removeEventListener('abort', kill);
-      child.once('close', remove);
+      child.once('exit', remove);
     }
   }
   return child;
diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js
@@ -5,48 +5,47 @@ const assert = require('assert');
 const exec = require('child_process').exec;
 const { promisify } = require('util');
 
-let pwdcommand, dir;
 const execPromisifed = promisify(exec);
 const invalidArgTypeError = {
   code: 'ERR_INVALID_ARG_TYPE',
   name: 'TypeError'
 };
 
-
+let waitCommand = '';
 if (common.isWindows) {
-  pwdcommand = 'echo %cd%';
-  dir = 'c:\\windows';
+  waitCommand = 'TIMEOUT 120';
 } else {
-  pwdcommand = 'pwd';
-  dir = '/dev';
+  waitCommand = 'sleep 2m';
 }
 
-
 {
   const ac = new AbortController();
   const signal = ac.signal;
-  const promise = execPromisifed(pwdcommand, { cwd: dir, signal });
-  assert.rejects(promise, /AbortError/).then(common.mustCall());
+  const promise = execPromisifed(waitCommand, { signal });
+  assert.rejects(promise, /AbortError/, 'post aborted sync signal failed')
+        .then(common.mustCall());
   ac.abort();
 }
 
 {
   assert.throws(() => {
-    execPromisifed(pwdcommand, { cwd: dir, signal: {} });
+    execPromisifed(waitCommand, { signal: {} });
   }, invalidArgTypeError);
 }
 
 {
   function signal() {}
   assert.throws(() => {
-    execPromisifed(pwdcommand, { cwd: dir, signal });
+    execPromisifed(waitCommand, { signal });
   }, invalidArgTypeError);
 }
 
 {
   const ac = new AbortController();
-  const signal = (ac.abort(), ac.signal);
-  const promise = execPromisifed(pwdcommand, { cwd: dir, signal });
+  const { signal } = ac;
+  ac.abort();
+  const promise = execPromisifed(waitCommand, { signal });
 
-  assert.rejects(promise, /AbortError/).then(common.mustCall());
+  assert.rejects(promise, /AbortError/, 'pre aborted signal failed')
+        .then(common.mustCall());
 }
diff --git a/test/parallel/test-child-process-fork-abort-signal.js b/test/parallel/test-child-process-fork-abort-signal.js
@@ -1,7 +1,7 @@
 // Flags: --experimental-abortcontroller
 'use strict';
 
-const { mustCall } = require('../common');
+const { mustCall, mustNotCall } = require('../common');
 const { strictEqual } = require('assert');
 const fixtures = require('../common/fixtures');
 const { fork } = require('child_process');
@@ -13,7 +13,10 @@ const { fork } = require('child_process');
   const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
     signal
   });
-  cp.on('exit', mustCall());
+  cp.on('exit', mustCall((code, killSignal) => {
+    strictEqual(code, null);
+    strictEqual(killSignal, 'SIGTERM');
+  }));
   cp.on('error', mustCall((err) => {
     strictEqual(err.name, 'AbortError');
   }));
@@ -27,8 +30,44 @@ const { fork } = require('child_process');
   const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
     signal
   });
-  cp.on('exit', mustCall());
+  cp.on('exit', mustCall((code, killSignal) => {
+    strictEqual(code, null);
+    strictEqual(killSignal, 'SIGTERM');
+  }));
+  cp.on('error', mustCall((err) => {
+    strictEqual(err.name, 'AbortError');
+  }));
+}
+
+{
+  // Test passing a different kill signal
+  const ac = new AbortController();
+  const { signal } = ac;
+  ac.abort();
+  const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
+    signal,
+    killSignal: 'SIGKILL',
+  });
+  cp.on('exit', mustCall((code, killSignal) => {
+    strictEqual(code, null);
+    strictEqual(killSignal, 'SIGKILL');
+  }));
   cp.on('error', mustCall((err) => {
     strictEqual(err.name, 'AbortError');
   }));
 }
+
+{
+  // Test aborting a cp before close but after exit
+  const ac = new AbortController();
+  const { signal } = ac;
+  const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
+    signal
+  });
+  cp.on('exit', mustCall(() => {
+    ac.abort();
+  }));
+  cp.on('error', mustNotCall());
+
+  setTimeout(() => cp.kill(), 1);
+}
diff --git a/test/parallel/test-child-process-spawn-controller.js b/test/parallel/test-child-process-spawn-controller.js
@@ -3,20 +3,25 @@
 
 const common = require('../common');
 const assert = require('assert');
-const cp = require('child_process');
+const { spawn } = require('child_process');
+const fixtures = require('../common/fixtures');
 
+const aliveScript = fixtures.path('child-process-stay-alive-forever.js');
 {
   // Verify that passing an AbortSignal works
   const controller = new AbortController();
   const { signal } = controller;
 
-  const echo = cp.spawn('echo', ['fun'], {
-    encoding: 'utf8',
-    shell: true,
-    signal
+  const cp = spawn(process.execPath, [aliveScript], {
+    signal,
   });
 
-  echo.on('error', common.mustCall((e) => {
+  cp.on('exit', common.mustCall((code, killSignal) => {
+    assert.strictEqual(code, null);
+    assert.strictEqual(killSignal, 'SIGTERM');
+  }));
+
+  cp.on('error', common.mustCall((e) => {
     assert.strictEqual(e.name, 'AbortError');
   }));
 
@@ -30,13 +35,76 @@ const cp = require('child_process');
 
   controller.abort();
 
-  const echo = cp.spawn('echo', ['fun'], {
-    encoding: 'utf8',
-    shell: true,
-    signal
+  const cp = spawn(process.execPath, [aliveScript], {
+    signal,
+  });
+  cp.on('exit', common.mustCall((code, killSignal) => {
+    assert.strictEqual(code, null);
+    assert.strictEqual(killSignal, 'SIGTERM');
+  }));
+
+  cp.on('error', common.mustCall((e) => {
+    assert.strictEqual(e.name, 'AbortError');
+  }));
+}
+
+{
+  // Verify that waiting a bit and closing works
+  const controller = new AbortController();
+  const { signal } = controller;
+
+  const cp = spawn(process.execPath, [aliveScript], {
+    signal,
+  });
+
+  cp.on('exit', common.mustCall((code, killSignal) => {
+    assert.strictEqual(code, null);
+    assert.strictEqual(killSignal, 'SIGTERM');
+  }));
+
+  cp.on('error', common.mustCall((e) => {
+    assert.strictEqual(e.name, 'AbortError');
+  }));
+
+  setTimeout(() => controller.abort(), 1);
+}
+
+{
+  // Test passing a different killSignal
+  const controller = new AbortController();
+  const { signal } = controller;
+
+  const cp = spawn(process.execPath, [aliveScript], {
+    signal,
+    killSignal: 'SIGKILL',
   });
 
-  echo.on('error', common.mustCall((e) => {
+  cp.on('exit', common.mustCall((code, killSignal) => {
+    assert.strictEqual(code, null);
+    assert.strictEqual(killSignal, 'SIGKILL');
+  }));
+
+  cp.on('error', common.mustCall((e) => {
     assert.strictEqual(e.name, 'AbortError');
   }));
+
+  setTimeout(() => controller.abort(), 1);
+}
+
+{
+  // Test aborting a cp before close but after exit
+  const controller = new AbortController();
+  const { signal } = controller;
+
+  const cp = spawn(process.execPath, [aliveScript], {
+    signal,
+  });
+
+  cp.on('exit', common.mustCall(() => {
+    controller.abort();
+  }));
+
+  cp.on('error', common.mustNotCall());
+
+  setTimeout(() => cp.kill(), 1);
 }
