child_process: refactor signal to use spawn function

nodejs · metcoder95 · Jul 7, 2020 · Jul 13, 2020 · Oct 23, 2020 · Oct 23, 2020
commit 9e3291e06a1053adffab5f738645f663bb7501fd
diff --git a/lib/child_process.js b/lib/child_process.js
@@ -58,12 +58,7 @@ let debug = require('internal/util/debuglog').debuglog(
     debug = fn;
   }
 );
-let DOMException;
-function lazyDOMException(message) {
-  if (DOMException === undefined)
-    DOMException = internalBinding('messaging').DOMException;
-  return new DOMException(message);
-}
+
 const { Buffer } = require('buffer');
 const { Pipe, constants: PipeConstants } = internalBinding('pipe_wrap');
 
@@ -191,77 +186,28 @@ function exec(command, options, callback) {
 }
 
 const customPromiseExecFunction = (orig) => {
-  return (file, options = {}) => {
+  return (...file) => {
     let child;
 
-    if (typeof options !== 'object' || options === null) {
-      return PromiseReject(
-        new ERR_INVALID_ARG_TYPE(
-          'options',
-          'Object',
-          options));
-    }
-
-    const { signal } = options;
-    const hasSignal = signal != null;
-
-    // Lets check if an AbortSignal is passed
-    if (hasSignal) {
-      const isNotValidSignal =
-        typeof signal !== 'object' || !('aborted' in signal);
-
-      if (isNotValidSignal) return PromiseReject(
-        new ERR_INVALID_ARG_TYPE(
-          'options.signal',
-          'AbortSignal',
-          signal));
-
-      // Already aborted
-      if (signal.aborted) return PromiseReject(lazyDOMException('AbortError'));
+    // Check if a callback is passed somehow
+    if (typeof file[file.length - 1] === 'function') {
+      file.pop();
     }
 
     const promise = new Promise((resolve, reject) => {
-      if (hasSignal) {
-        signal.addEventListener(
-          'abort',
-          rejectOnAbortSignal(reject),
-          { once: true }
-        );
-      }
-
-      child = orig(file, options, (err, stdout, stderr) => {
-        if (hasSignal && signal.aborted) {
-          reject(err);
-        } else if (err !== null) {
+      child = orig(...file, (err, stdout, stderr) => {
+        if (err !== null) {
           err.stdout = stdout;
           err.stderr = stderr;
           reject(err);
         } else {
           resolve({ stdout, stderr });
         }
-
-        // Let's cleanup once exec is finished
-        if (hasSignal)
-          signal
-            .removeEventListener(
-              'abort',
-              rejectOnAbortSignal(reject),
-              { once: true }
-            );
       });
     });
 
     promise.child = child;
 
-    function rejectOnAbortSignal(reject) {
-      return () => {
-        if (promise && promise.child)
-          promise.child.kill(options.killSignal || 'SIGTERM');
-
-        reject(lazyDOMException('AbortError'));
-      };
-    }
-
     return promise;
   };
 };
@@ -315,9 +261,6 @@ function execFile(file /* , args, options, callback */) {
   // Validate maxBuffer, if present.
   validateMaxBuffer(options.maxBuffer);
 
-  // Validate signal, if present
-  validateAbortSignal(options.signal, 'options.signal');
-
   options.killSignal = sanitizeKillSignal(options.killSignal);
 
   const child = spawn(file, args, {
@@ -327,7 +270,8 @@ function execFile(file /* , args, options, callback */) {
     uid: options.uid,
     shell: options.shell,
     windowsHide: !!options.windowsHide,
-    windowsVerbatimArguments: !!options.windowsVerbatimArguments
+    windowsVerbatimArguments: !!options.windowsVerbatimArguments,
+    signal: options.signal
   });
 
   let encoding;
@@ -429,28 +373,12 @@ function execFile(file /* , args, options, callback */) {
     }
   }
 
-  function abortHandler() {
-    if (!ex)
-      ex = new AbortError();
-    process.nextTick(() => kill());
-  }
-
   if (options.timeout > 0) {
     timeoutId = setTimeout(function delayedKill() {
       kill();
       timeoutId = null;
     }, options.timeout);
   }
-  if (options.signal) {
-    if (options.signal.aborted) {
-      process.nextTick(abortHandler);
-    } else {
-      const childController = new AbortController();
-      options.signal.addEventListener('abort', abortHandler,
-                                      { signal: childController.signal });
-      child.once('close', () => childController.abort());
-    }
-  }
 
   if (child.stdout) {
     if (encoding)
@@ -674,10 +602,43 @@ function normalizeSpawnArguments(file, args, options) {
 
 function spawn(file, args, options) {
   const child = new ChildProcess();
-
   options = normalizeSpawnArguments(file, args, options);
+
+  if (options.signal) {
+    const signal = options.signal;
+    // Validate signal, if present
+    validateAbortSignal(signal, 'options.signal');
+
+    const onAbortListener = onAbortSignal(child);
+
+    // Do nothing and throw if already aborted
+    if (signal.aborted) {
+      onAbortListener();
+    } else {
+      signal.addEventListener('abort', onAbortListener, { once: true });
+      child.once('close', () => signal.removeEventListener('abort', onAbortListener, { once: true }));
+    }
+
+    function onAbortSignal(childProcess) {
+      return function abortListener() {
+        if (childProcess.stdout) childProcess.stdout.destroy();
+
+        if (childProcess.stderr) childProcess.stderr.destroy();
+
+        process.nextTick(() => {
+          if (childProcess && childProcess.kill) {
+            childProcess.kill(options.killSignal);
+          }
+
+          childProcess.emit('error', new AbortError());
+        });
+      }
+    }
+  }
+
   debug('spawn', options);
   child.spawn(options);
+
 
   return child;
 }

diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js
@@ -22,7 +22,6 @@ if (common.isWindows) {
   const promise = execPromisifed(pwdcommand, { cwd: dir, signal });
   assert.rejects(promise, /AbortError/);
   ac.abort();
-  assert.strictEqual(promise.child.killed, true);
 }
 
 {
@@ -50,5 +49,4 @@ if (common.isWindows) {
   const promise = execPromisifed(pwdcommand, { cwd: dir, signal });
 
   assert.rejects(promise, /AbortError/);
-  assert.strictEqual(promise.child, undefined);
 }
diff --git a/test/parallel/test-child-process-execFile-promisified-abortController.js b/test/parallel/test-child-process-execFile-promisified-abortController.js
@@ -0,0 +1,63 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const { promisify } = require('util');
+const execFile = require('child_process').execFile;
+const { getSystemErrorName } = require('util');
+const fixtures = require('../common/fixtures');
+
+const fixture = fixtures.path('exit.js');
+const echoFixture = fixtures.path('echo.js');
+const execOpts = { encoding: 'utf8', shell: true };
+const execFilePromisified = promisify(execFile);
+
+{
+  // Verify that the signal option works properly
+  const ac = new AbortController();
+  const signal = ac.signal;
+  const promise = execFilePromisified(process.execPath, [echoFixture, 0], { signal });
+
+  ac.abort();
+
+  assert.rejects(promise, /AbortError/);
+}
+
+{
+  // Verify that the signal option works properly when already aborted
+  const ac = new AbortController();
+  const { signal } = ac;
+  ac.abort();
+
+  const promise = execFilePromisified(process.execPath, [echoFixture, 0], { signal });
+
+  assert.rejects(promise, /AbortError/);
+}
+
+{
+  // Verify that if something different than Abortcontroller.signal
+  // is passed, ERR_INVALID_ARG_TYPE is thrown
+  const promise = execFilePromisified(process.execPath, [echoFixture, 0], { signal: {} });
+
+  assert.rejects(promise, {
+    code: 'ERR_INVALID_ARG_TYPE',
+    name: 'TypeError',
+    message: 'The "options.signal" property must be an ' +
+             'instance of AbortSignal. Received an instance of Object'
+  });
+
+}
+
+{
+  // Verify that if something different than Abortcontroller.signal
+  // is passed, ERR_INVALID_ARG_TYPE is thrown
+  const promise = execFilePromisified(process.execPath, [echoFixture, 0], { signal: 'world!' });
+
+  assert.rejects(promise, {
+    code: 'ERR_INVALID_ARG_TYPE',
+    name: 'TypeError',
+    message: 'The "options.signal" property must be an ' +
+             `instance of AbortSignal. Received type string ('world!')`
+  });
+
+}
diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js
@@ -53,21 +53,32 @@ const execOpts = { encoding: 'utf8', shell: true };
   const ac = new AbortController();
   const { signal } = ac;
 
-  const test = () => {
-    const check = common.mustCall((err) => {
-      assert.strictEqual(err.code, 'ABORT_ERR');
-      assert.strictEqual(err.name, 'AbortError');
-      assert.strictEqual(err.signal, undefined);
-    });
-    execFile(process.execPath, [echoFixture, 0], { signal }, check);
-  };
+  const callback = common.mustCall((err) => {
+    assert.strictEqual(err.code, 'ABORT_ERR');
+    assert.strictEqual(err.name, 'AbortError');
+  });
 
-  test();
+  execFile(process.execPath, [echoFixture, 0], { signal }, callback);
   ac.abort();
   // Verify that it still works the same way now that the signal is aborted.
   test();
 }
 
+{
+  // Verify that does not spawn a child if already aborted
+  const ac = new AbortController();
+  const { signal } = ac;
+  ac.abort();
+
+  try {
+      execFile(process.execPath, [echoFixture, 0], { signal }); 
+  } catch (err) {
+      assert.strictEqual(err.code, 'ABORT_ERR');
+      assert.strictEqual(err.name, 'AbortError');
+  }
+
+}
+
 {
   // Verify that if something different than Abortcontroller.signal
   // is passed, ERR_INVALID_ARG_TYPE is thrown