From 0bcaf9c4d172f396206ee3b534527a4799782b7b Mon Sep 17 00:00:00 2001
From: Nitzan Uziely <nitzan@testim.io>
Date: Thu, 11 Feb 2021 20:59:00 +0200
Subject: [PATCH] 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: https://github.com/nodejs/node/issues/37273

PR-URL: https://github.com/nodejs/node/pull/37325
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
---
 doc/api/child_process.md                      | 10 +++
 lib/child_process.js                          | 33 ++++---
 ...rocess-exec-abortcontroller-promisified.js | 27 +++---
 .../test-child-process-fork-abort-signal.js   | 45 +++++++++-
 .../test-child-process-spawn-controller.js    | 90 ++++++++++++++++---
 5 files changed, 165 insertions(+), 40 deletions(-)

diff --git a/doc/api/child_process.md b/doc/api/child_process.md
index e3151b78e91957..6ebf17c7b29482 100644
--- 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
index e9c52eef7e21ba..2ef9dcba43c10b 100644
--- 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
index 957e858d004c52..2db880906437e3 100644
--- 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
index 809821738d1b9d..817366a71f2bcc 100644
--- 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
index 07f9cef3557d75..d76b4391870c26 100644
--- 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);
 }