From d78643eb17c5a365711e7e43dac2f6cc6caf6d32 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
---
 doc/api/child_process.md                      | 10 +++
 lib/child_process.js                          | 33 ++++---
 .../test-child-process-fork-abort-signal.js   | 45 +++++++++-
 .../test-child-process-spawn-controller.js    | 90 ++++++++++++++++---
 4 files changed, 152 insertions(+), 26 deletions(-)

diff --git a/doc/api/child_process.md b/doc/api/child_process.md
index 87a62ddbbc864e..d69ba334154d0a 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: v15.6.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: v15.5.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 4325caa9496eb1..f3f1a10af0e615 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -586,6 +586,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);
+  }
+}
+
 
 function spawn(file, args, options) {
   const child = new ChildProcess();
@@ -595,21 +607,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);
       });
     }
   }
@@ -765,19 +775,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-fork-abort-signal.js b/test/parallel/test-child-process-fork-abort-signal.js
index 63367479234d7f..93a968366270cc 100644
--- a/test/parallel/test-child-process-fork-abort-signal.js
+++ b/test/parallel/test-child-process-fork-abort-signal.js
@@ -1,6 +1,6 @@
 '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');
@@ -12,7 +12,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');
   }));
@@ -26,8 +29,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 c25307907c1b26..36c4112cbe00da 100644
--- a/test/parallel/test-child-process-spawn-controller.js
+++ b/test/parallel/test-child-process-spawn-controller.js
@@ -2,20 +2,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');
   }));
 
@@ -29,13 +34,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);
 }