diff --git a/lib/child_process.js b/lib/child_process.js index 7e3e5afe140478..8c686c074c6bf0 100644 --- a/lib/child_process.js +++ b/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()); } } 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..957e858d004c52 --- /dev/null +++ b/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()); +} 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..b2724eeb01be7a --- /dev/null +++ b/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); +} diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index 28bb498ea4b51d..f75dd05f81a386 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/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); } {