From b68b13acb3e161735a2845c26218227d35b670da Mon Sep 17 00:00:00 2001
From: Nitzan Uziely <nitzan@testim.io>
Date: Sun, 7 Feb 2021 01:39:54 +0200
Subject: [PATCH] child_process: add timeout to spawn and fork

Add support for timeout to spawn and fork.

Fixes: https://github.com/nodejs/node/issues/27639

PR-URL: https://github.com/nodejs/node/pull/37256
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
---
 doc/api/child_process.md                      | 18 +++++--
 lib/child_process.js                          | 29 ++++++++--
 ...-child-process-fork-timeout-kill-signal.js | 54 +++++++++++++++++++
 ...child-process-spawn-timeout-kill-signal.js | 51 ++++++++++++++++++
 4 files changed, 143 insertions(+), 9 deletions(-)
 create mode 100644 test/parallel/test-child-process-fork-timeout-kill-signal.js
 create mode 100644 test/parallel/test-child-process-spawn-timeout-kill-signal.js

diff --git a/doc/api/child_process.md b/doc/api/child_process.md
index 4b6215d12b1f89..d277cda93ba474 100644
--- a/doc/api/child_process.md
+++ b/doc/api/child_process.md
@@ -374,6 +374,9 @@ controller.abort();
 <!-- YAML
 added: v0.5.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/37256
+    description: timeout was added.
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/37325
     description: killSignal for AbortSignal was added.
@@ -410,8 +413,8 @@ changes:
     See [Advanced serialization][] for more details. **Default:** `'json'`.
   * `signal` {AbortSignal} Allows closing the child process using an
     AbortSignal.
-  * `killSignal` {string} The signal value to be used when the spawned
-    process will be killed by the abort signal. **Default:** `'SIGTERM'`.
+  * `killSignal` {string|integer} The signal value to be used when the spawned
+    process will be killed by timeout or 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
@@ -423,6 +426,8 @@ changes:
   * `uid` {number} Sets the user identity of the process (see setuid(2)).
   * `windowsVerbatimArguments` {boolean} No quoting or escaping of arguments is
     done on Windows. Ignored on Unix. **Default:** `false`.
+  * `timeout` {number} In milliseconds the maximum amount of time the process
+    is allowed to run. **Default:** `undefined`.
 * Returns: {ChildProcess}
 
 The `child_process.fork()` method is a special case of
@@ -478,6 +483,9 @@ if (process.argv[2] === 'child') {
 <!-- YAML
 added: v0.1.90
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/37256
+    description: timeout was added.
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/37325
     description: killSignal for AbortSignal was added.
@@ -528,8 +536,10 @@ changes:
     normally be created on Windows systems. **Default:** `false`.
   * `signal` {AbortSignal} allows aborting the child process using an
     AbortSignal.
-  * `killSignal` {string} The signal value to be used when the spawned
-    process will be killed by the abort signal. **Default:** `'SIGTERM'`.
+  * `timeout` {number} In milliseconds the maximum amount of time the process
+    is allowed to run. **Default:** `undefined`.
+  * `killSignal` {string|integer} The signal value to be used when the spawned
+    process will be killed by timeout or abort signal. **Default:** `'SIGTERM'`.
 
 * Returns: {ChildProcess}
 
diff --git a/lib/child_process.js b/lib/child_process.js
index 2ef9dcba43c10b..6de743558a5611 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -646,15 +646,14 @@ function abortChildProcess(child, killSignal) {
  * @returns {ChildProcess}
  */
 function spawn(file, args, options) {
-  const child = new ChildProcess();
   options = normalizeSpawnArguments(file, args, options);
+  validateTimeout(options.timeout, 'options.timeout');
+  validateAbortSignal(options.signal, 'options.signal');
+  const killSignal = sanitizeKillSignal(options.killSignal);
+  const child = new ChildProcess();
 
   if (options.signal) {
     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 {
@@ -673,6 +672,26 @@ function spawn(file, args, options) {
   debug('spawn', options);
   child.spawn(options);
 
+  if (options.timeout > 0) {
+    let timeoutId = setTimeout(() => {
+      if (timeoutId) {
+        try {
+          child.kill(killSignal);
+        } catch (err) {
+          child.emit('error', err);
+        }
+        timeoutId = null;
+      }
+    }, options.timeout);
+
+    child.once('exit', () => {
+      if (timeoutId) {
+        clearTimeout(timeoutId);
+        timeoutId = null;
+      }
+    });
+  }
+
   return child;
 }
 
diff --git a/test/parallel/test-child-process-fork-timeout-kill-signal.js b/test/parallel/test-child-process-fork-timeout-kill-signal.js
new file mode 100644
index 00000000000000..f5b9bd281f310b
--- /dev/null
+++ b/test/parallel/test-child-process-fork-timeout-kill-signal.js
@@ -0,0 +1,54 @@
+// Flags: --expose-internals
+'use strict';
+
+const { mustCall } = require('../common');
+const { strictEqual, throws } = require('assert');
+const fixtures = require('../common/fixtures');
+const { fork } = require('child_process');
+const { getEventListeners } = require('events');
+const {
+  EventTarget,
+} = require('internal/event_target');
+
+{
+  // Verify default signal
+  const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
+    timeout: 5,
+  });
+  cp.on('exit', mustCall((code, ks) => strictEqual(ks, 'SIGTERM')));
+}
+
+{
+  // Verify correct signal + closes after at least 4 ms.
+  const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
+    timeout: 5,
+    killSignal: 'SIGKILL',
+  });
+  cp.on('exit', mustCall((code, ks) => strictEqual(ks, 'SIGKILL')));
+}
+
+{
+  // Verify timeout verification
+  throws(() => fork(fixtures.path('child-process-stay-alive-forever.js'), {
+    timeout: 'badValue',
+  }), /ERR_OUT_OF_RANGE/);
+
+  throws(() => fork(fixtures.path('child-process-stay-alive-forever.js'), {
+    timeout: {},
+  }), /ERR_OUT_OF_RANGE/);
+}
+
+{
+  // Verify abort signal gets unregistered
+  const signal = new EventTarget();
+  signal.aborted = false;
+
+  const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
+    timeout: 6,
+    signal,
+  });
+  strictEqual(getEventListeners(signal, 'abort').length, 1);
+  cp.on('exit', mustCall(() => {
+    strictEqual(getEventListeners(signal, 'abort').length, 0);
+  }));
+}
diff --git a/test/parallel/test-child-process-spawn-timeout-kill-signal.js b/test/parallel/test-child-process-spawn-timeout-kill-signal.js
new file mode 100644
index 00000000000000..f0bedebc597a1d
--- /dev/null
+++ b/test/parallel/test-child-process-spawn-timeout-kill-signal.js
@@ -0,0 +1,51 @@
+// Flags: --experimental-abortcontroller
+'use strict';
+
+const { mustCall } = require('../common');
+const { strictEqual, throws } = require('assert');
+const fixtures = require('../common/fixtures');
+const { spawn } = require('child_process');
+const { getEventListeners } = require('events');
+
+const aliveForeverFile = 'child-process-stay-alive-forever.js';
+{
+  // Verify default signal + closes
+  const cp = spawn(process.execPath, [fixtures.path(aliveForeverFile)], {
+    timeout: 5,
+  });
+  cp.on('exit', mustCall((code, ks) => strictEqual(ks, 'SIGTERM')));
+}
+
+{
+  // Verify SIGKILL signal + closes
+  const cp = spawn(process.execPath, [fixtures.path(aliveForeverFile)], {
+    timeout: 6,
+    killSignal: 'SIGKILL',
+  });
+  cp.on('exit', mustCall((code, ks) => strictEqual(ks, 'SIGKILL')));
+}
+
+{
+  // Verify timeout verification
+  throws(() => spawn(process.execPath, [fixtures.path(aliveForeverFile)], {
+    timeout: 'badValue',
+  }), /ERR_OUT_OF_RANGE/);
+
+  throws(() => spawn(process.execPath, [fixtures.path(aliveForeverFile)], {
+    timeout: {},
+  }), /ERR_OUT_OF_RANGE/);
+}
+
+{
+  // Verify abort signal gets unregistered
+  const controller = new AbortController();
+  const { signal } = controller;
+  const cp = spawn(process.execPath, [fixtures.path(aliveForeverFile)], {
+    timeout: 6,
+    signal,
+  });
+  strictEqual(getEventListeners(signal, 'abort').length, 1);
+  cp.on('exit', mustCall(() => {
+    strictEqual(getEventListeners(signal, 'abort').length, 0);
+  }));
+}