From 2242cbb25343470ef92263f4bc6f0e35d61d8d17 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Tue, 7 Jul 2020 23:14:00 +0200 Subject: [PATCH] child_process: allow promisified exec to be cancel Using new AbortController, add support for promisified exec to be cancelled. PR-URL: https://github.com/nodejs/node/pull/34249 Reviewed-By: Antoine du Hamel Reviewed-By: James M Snell --- lib/child_process.js | 58 +++++++++++-------- ...rocess-exec-abortcontroller-promisified.js | 51 ++++++++++++++++ ...ss-execFile-promisified-abortController.js | 58 +++++++++++++++++++ test/parallel/test-child-process-execfile.js | 17 +++++- 4 files changed, 158 insertions(+), 26 deletions(-) create mode 100644 test/parallel/test-child-process-exec-abortcontroller-promisified.js create mode 100644 test/parallel/test-child-process-execFile-promisified-abortController.js diff --git a/lib/child_process.js b/lib/child_process.js index 42d197342bc8ab..f51072126e21cd 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -140,7 +140,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) { @@ -254,17 +254,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 }); @@ -368,28 +366,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) @@ -611,8 +593,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); @@ -752,14 +757,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()); } } diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js new file mode 100644 index 00000000000000..fa00f2d2177635 --- /dev/null +++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js @@ -0,0 +1,51 @@ +'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()); +} diff --git a/test/parallel/test-child-process-execFile-promisified-abortController.js b/test/parallel/test-child-process-execFile-promisified-abortController.js new file mode 100644 index 00000000000000..ccfcafc5d51e58 --- /dev/null +++ b/test/parallel/test-child-process-execFile-promisified-abortController.js @@ -0,0 +1,58 @@ +'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); +} diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index 49a4bdbda29957..a8f0cabacc2860 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -62,10 +62,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); } {