child_process: fix spawn and fork abort behavior

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>
nodejs · Sep 1, 2021 · 0bcaf9c · 0bcaf9c
1 parent 8010c83
commit 0bcaf9c
Show file tree

Hide file tree

Showing 5 changed files with 165 additions and 40 deletions.
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);
 }