child_process: allow promisified exec to be cancel

Using new AbortController, add support for promisified
exec to be cancelled.

PR-URL: nodejs#34249
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: metcoder95

lib/child_process.js

@@ -43,7 +43,6 @@ let debug = require('internal/util/debuglog').debuglog(
     debug = fn;
   }
 );
-const { AbortController } = require('internal/abort_controller');
 const { Buffer } = require('buffer');
 const { Pipe, constants: PipeConstants } = internalBinding('pipe_wrap');
 
@@ -150,7 +149,7 @@ function fork(modulePath /* , args, options */) {
   options.execPath = options.execPath || process.execPath;
   options.shell = false;
 
-  return spawnWithSignal(options.execPath, args, options);
+  return spawn(options.execPath, args, options);
 }
 
 function _forkChild(fd, serializationMode) {
@@ -311,17 +310,15 @@ 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, {
     cwd: options.cwd,
     env: options.env,
     gid: options.gid,
-    uid: options.uid,
     shell: options.shell,
+    signal: options.signal,
+    uid: options.uid,
     windowsHide: !!options.windowsHide,
     windowsVerbatimArguments: !!options.windowsVerbatimArguments
   });
@@ -425,28 +422,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)
@@ -661,8 +642,31 @@ 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');
+
+    // 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));
+    }
+
+    function onAbortListener() {
+      process.nextTick(() => {
+        child?.kill?.(options.killSignal);
+
+        child.emit('error', new AbortError());
+      });
+    }
+  }
+
   debug('spawn', options);
   child.spawn(options);
 
@@ -868,14 +872,19 @@ function sanitizeKillSignal(killSignal) {
 // This level of indirection is here because the other child_process methods
 // call spawn internally but should use different cancellation logic.
 function spawnWithSignal(file, args, options) {
-  const child = spawn(file, args, options);
+  // Remove signal from options to spawn
+  // to avoid double emitting of AbortError
+  const opts = options && typeof options === 'object' && ('signal' in options) ?
+    { ...options, signal: undefined } :
+    options;
+  const child = spawn(file, args, opts);
 
   if (options && options.signal) {
     // Validate signal, if present
     validateAbortSignal(options.signal, 'options.signal');
     function kill() {
       if (child._handle) {
-        child.kill('SIGTERM');
+        child._handle.kill(options.killSignal || 'SIGTERM');
         child.emit('error', new AbortError());
       }
     }

test/parallel/test-child-process-exec-abortcontroller-promisified.js

@@ -0,0 +1,52 @@
+// Flags: --experimental-abortcontroller
+'use strict';
+const common = require('../common');
+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'
+};
+
+
+if (common.isWindows) {
+  pwdcommand = 'echo %cd%';
+  dir = 'c:\\windows';
+} else {
+  pwdcommand = 'pwd';
+  dir = '/dev';
+}
+
+
+{
+  const ac = new AbortController();
+  const signal = ac.signal;
+  const promise = execPromisifed(pwdcommand, { cwd: dir, signal });
+  assert.rejects(promise, /AbortError/).then(common.mustCall());
+  ac.abort();
+}
+
+{
+  assert.throws(() => {
+    execPromisifed(pwdcommand, { cwd: dir, signal: {} });
+  }, invalidArgTypeError);
+}
+
+{
+  function signal() {}
+  assert.throws(() => {
+    execPromisifed(pwdcommand, { cwd: dir, signal });
+  }, invalidArgTypeError);
+}
+
+{
+  const ac = new AbortController();
+  const signal = (ac.abort(), ac.signal);
+  const promise = execPromisifed(pwdcommand, { cwd: dir, signal });
+
+  assert.rejects(promise, /AbortError/).then(common.mustCall());
+}

test/parallel/test-child-process-execFile-promisified-abortController.js

@@ -0,0 +1,59 @@
+// Flags: --experimental-abortcontroller
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const { promisify } = require('util');
+const execFile = require('child_process').execFile;
+const fixtures = require('../common/fixtures');
+
+const echoFixture = fixtures.path('echo.js');
+const promisified = promisify(execFile);
+const invalidArgTypeError = {
+  code: 'ERR_INVALID_ARG_TYPE',
+  name: 'TypeError'
+};
+
+{
+  // Verify that the signal option works properly
+  const ac = new AbortController();
+  const signal = ac.signal;
+  const promise = promisified(process.execPath, [echoFixture, 0], { signal });
+
+  ac.abort();
+
+  assert.rejects(
+    promise,
+    { name: 'AbortError' }
+  ).then(common.mustCall());
+}
+
+{
+  // Verify that the signal option works properly when already aborted
+  const ac = new AbortController();
+  const { signal } = ac;
+  ac.abort();
+
+  assert.rejects(
+    promisified(process.execPath, [echoFixture, 0], { signal }),
+    { name: 'AbortError' }
+  ).then(common.mustCall());
+}
+
+{
+  // Verify that if something different than Abortcontroller.signal
+  // is passed, ERR_INVALID_ARG_TYPE is thrown
+  const signal = {};
+  assert.throws(() => {
+    promisified(process.execPath, [echoFixture, 0], { signal });
+  }, invalidArgTypeError);
+}
+
+{
+  // Verify that if something different than Abortcontroller.signal
+  // is passed, ERR_INVALID_ARG_TYPE is thrown
+  const signal = 'world!';
+  assert.throws(() => {
+    promisified(process.execPath, [echoFixture, 0], { signal });
+  }, invalidArgTypeError);
+}

test/parallel/test-child-process-execfile.js

@@ -63,10 +63,23 @@ const execOpts = { encoding: 'utf8', shell: true };
     execFile(process.execPath, [echoFixture, 0], { signal }, check);
   };
 
-  test();
-  ac.abort();
   // Verify that it still works the same way now that the signal is aborted.
   test();
+  ac.abort();
+}
+
+{
+  // Verify that does not spawn a child if already aborted
+  const ac = new AbortController();
+  const { signal } = ac;
+  ac.abort();
+
+  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);
 }
 
 {