From eed3f17252f0da5a823e6a68d6e4a1a656a2d44a Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Tue, 7 Jul 2020 23:14:00 +0200 Subject: [PATCH 01/25] child_process: allow promisified exec to be cancel Using new experimental AbortController, add support for promisified exec to be cancelled. --- lib/child_process.js | 34 +++++++++- ...rocess-exec-abortcontroller-promisified.js | 64 +++++++++++++++++++ 2 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-child-process-exec-abortcontroller-promisified.js diff --git a/lib/child_process.js b/lib/child_process.js index daa1d44e8974df..687034f733b3bf 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -49,6 +49,7 @@ const { convertToValidSignal, getSystemErrorName } = require('internal/util'); + const { isArrayBufferView } = require('internal/util/types'); let debug = require('internal/util/debuglog').debuglog( 'child_process', @@ -56,6 +57,12 @@ 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'); @@ -183,7 +190,7 @@ function exec(command, options, callback) { } const customPromiseExecFunction = (orig) => { - return (...args) => { + return (file, options = {}) => { let resolve; let reject; const promise = new Promise((res, rej) => { @@ -191,7 +198,30 @@ const customPromiseExecFunction = (orig) => { reject = rej; }); - promise.child = orig(...args, (err, stdout, stderr) => { + if (options == null || typeof options !== 'object') { + return Promise.reject( + new ERR_INVALID_ARG_TYPE( + 'options', + 'Object', + options)); + } + + const { signal } = options; + if (signal == null || + typeof signal !== 'object' || + !('aborted' in signal)) { + return Promise.reject( + new ERR_INVALID_ARG_TYPE( + 'options.signal', + 'AbortSignal', + signal)); + } + + signal.addEventListener('abort', () => { + reject(lazyDOMException('AbortError')); + }, { once: true }); + + promise.child = orig(file, options, (err, stdout, stderr) => { if (err !== null) { err.stdout = stdout; err.stderr = stderr; 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..6432c1d6933cc9 --- /dev/null +++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js @@ -0,0 +1,64 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +'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); + +if (common.isWindows) { + pwdcommand = 'echo %cd%'; + dir = 'c:\\windows'; +} else { + pwdcommand = 'pwd'; + dir = '/dev'; +} + + +{ + const ac = new AbortController(); + const signal = ac.signal; + assert.rejects(execPromisifed(pwdcommand, { cwd: dir, signal }), /AbortError/); + ac.abort(); +} + +{ + assert.rejects(execPromisifed(pwdcommand, { cwd: dir, signal: {} }), { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: 'The "options.signal" property must be an ' + + 'instance of AbortSignal. Received an instance of Object' + }); +} + +{ + function signal() {} + assert.rejects(execPromisifed(pwdcommand, { cwd: dir, signal }), { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: 'The "options.signal" property must be an ' + + 'instance of AbortSignal. Received function signal' + }); +} From 0ef841d6d968f9f047003ae1027d32836b681f14 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Tue, 14 Jul 2020 00:04:39 +0200 Subject: [PATCH 02/25] child_process: fix validation issue When doing a validation for the signal object, the validation was assuming wrongly that the signal object will always be there to apply the correct event-listener. The fix does a better validation to avoid fall in "addEventListener is not a function" --- lib/child_process.js | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 687034f733b3bf..8b3cea3fa57e4f 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -198,7 +198,7 @@ const customPromiseExecFunction = (orig) => { reject = rej; }); - if (options == null || typeof options !== 'object') { + if (typeof options !== 'object') { return Promise.reject( new ERR_INVALID_ARG_TYPE( 'options', @@ -207,19 +207,21 @@ const customPromiseExecFunction = (orig) => { } const { signal } = options; - if (signal == null || - typeof signal !== 'object' || - !('aborted' in signal)) { - return Promise.reject( + const hasSignal = signal != null; + const isNotValidSignal = typeof signal !== 'object' || + !('aborted' in signal); + + if (hasSignal) { + if (isNotValidSignal) return Promise.reject( new ERR_INVALID_ARG_TYPE( 'options.signal', 'AbortSignal', signal)); - } - signal.addEventListener('abort', () => { - reject(lazyDOMException('AbortError')); - }, { once: true }); + signal.addEventListener('abort', () => { + reject(lazyDOMException('AbortError')); + }, { once: true }); + } promise.child = orig(file, options, (err, stdout, stderr) => { if (err !== null) { From 70cb86cd261ec25b2e8326cf05e70c7471fb30ac Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 23 Oct 2020 16:37:23 +0200 Subject: [PATCH 03/25] child_process: use Primordials instead of plain Promise --- lib/child_process.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 8b3cea3fa57e4f..7e56ee30020de2 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -42,6 +42,7 @@ const { SafeSet, StringPrototypeSlice, StringPrototypeToUpperCase, + PromiseReject, } = primordials; const { @@ -198,8 +199,8 @@ const customPromiseExecFunction = (orig) => { reject = rej; }); - if (typeof options !== 'object') { - return Promise.reject( + if (typeof options !== 'object' || options === null) { + return PromiseReject( new ERR_INVALID_ARG_TYPE( 'options', 'Object', @@ -212,15 +213,16 @@ const customPromiseExecFunction = (orig) => { !('aborted' in signal); if (hasSignal) { - if (isNotValidSignal) return Promise.reject( + const isNotValidSignal = typeof signal !== 'object' || !('aborted' in signal); + + if (isNotValidSignal) return PromiseReject( new ERR_INVALID_ARG_TYPE( 'options.signal', 'AbortSignal', signal)); - signal.addEventListener('abort', () => { - reject(lazyDOMException('AbortError')); - }, { once: true }); + // Already aborted + if (signal.aborted) return PromiseReject(lazyDOMException('AbortError')); } promise.child = orig(file, options, (err, stdout, stderr) => { From a6ae623ddd5c0ecd5799378b9720abe408f6ce6f Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 23 Oct 2020 16:37:56 +0200 Subject: [PATCH 04/25] child_process: refactor according to review --- lib/child_process.js | 43 ++++++++++++------- ...rocess-exec-abortcontroller-promisified.js | 6 +++ 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 7e56ee30020de2..cbae17edc94630 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -192,12 +192,7 @@ function exec(command, options, callback) { const customPromiseExecFunction = (orig) => { return (file, options = {}) => { - let resolve; - let reject; - const promise = new Promise((res, rej) => { - resolve = res; - reject = rej; - }); + let child; if (typeof options !== 'object' || options === null) { return PromiseReject( @@ -209,9 +204,8 @@ const customPromiseExecFunction = (orig) => { const { signal } = options; const hasSignal = signal != null; - const isNotValidSignal = typeof signal !== 'object' || - !('aborted' in signal); + // Lets check if an AbortSignal is passed if (hasSignal) { const isNotValidSignal = typeof signal !== 'object' || !('aborted' in signal); @@ -225,16 +219,33 @@ const customPromiseExecFunction = (orig) => { if (signal.aborted) return PromiseReject(lazyDOMException('AbortError')); } - promise.child = orig(file, options, (err, stdout, stderr) => { - if (err !== null) { - err.stdout = stdout; - err.stderr = stderr; - reject(err); - } else { - resolve({ stdout, stderr }); - } + const promise = new Promise((resolve, reject) => { + if (hasSignal) signal.addEventListener('abort', rejectOnAbortSignal(reject), { once: true }); + + child = orig(file, options, (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; }; }; diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js index 6432c1d6933cc9..d5afe928c245af 100644 --- a/test/parallel/test-child-process-exec-abortcontroller-promisified.js +++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js @@ -62,3 +62,9 @@ if (common.isWindows) { 'instance of AbortSignal. Received function signal' }); } + +{ + const ac = new AbortController(); + const signal = (ac.abort(), ac.signal); + assert.rejects(execPromisifed(pwdcommand, { cwd: dir, signal }), /AbortError/); +} From 988ea3fb9c4622c1ecc3b6514bb36d9110a9aa9d Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 23 Oct 2020 16:38:11 +0200 Subject: [PATCH 05/25] child_process: remove license from test --- ...rocess-exec-abortcontroller-promisified.js | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js index d5afe928c245af..271903c5f196f9 100644 --- a/test/parallel/test-child-process-exec-abortcontroller-promisified.js +++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js @@ -1,24 +1,3 @@ -// Copyright Joyent, Inc. and other Node contributors. -// -// Permission is hereby granted, free of charge, to any person obtaining a -// copy of this software and associated documentation files (the -// "Software"), to deal in the Software without restriction, including -// without limitation the rights to use, copy, modify, merge, publish, -// distribute, sublicense, and/or sell copies of the Software, and to permit -// persons to whom the Software is furnished to do so, subject to the -// following conditions: -// -// The above copyright notice and this permission notice shall be included -// in all copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS -// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF -// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN -// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, -// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR -// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE -// USE OR OTHER DEALINGS IN THE SOFTWARE. - 'use strict'; const common = require('../common'); const assert = require('assert'); From 78e75219b85e50d7c39bed0c3887951d3c038812 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 23 Oct 2020 16:54:51 +0200 Subject: [PATCH 06/25] child_process: improve testing --- ...est-child-process-exec-abortcontroller-promisified.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js index 271903c5f196f9..3eac82ebfb87d6 100644 --- a/test/parallel/test-child-process-exec-abortcontroller-promisified.js +++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js @@ -19,8 +19,10 @@ if (common.isWindows) { { const ac = new AbortController(); const signal = ac.signal; - assert.rejects(execPromisifed(pwdcommand, { cwd: dir, signal }), /AbortError/); + const promise = execPromisifed(pwdcommand, { cwd: dir, signal }); + assert.rejects(promise, /AbortError/); ac.abort(); + assert.strictEqual(promise.child.killed, true); } { @@ -45,5 +47,8 @@ if (common.isWindows) { { const ac = new AbortController(); const signal = (ac.abort(), ac.signal); - assert.rejects(execPromisifed(pwdcommand, { cwd: dir, signal }), /AbortError/); + const promise = execPromisifed(pwdcommand, { cwd: dir, signal }); + + assert.rejects(promise, /AbortError/); + assert.strictEqual(promise.child, undefined); } From c8210f66c9bac7fbb3fd7cf2f3deeeed78768461 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 23 Oct 2020 17:40:33 +0200 Subject: [PATCH 07/25] child_process: fix linter issues --- lib/child_process.js | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index cbae17edc94630..b5687fb7c53f16 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -207,7 +207,8 @@ const customPromiseExecFunction = (orig) => { // Lets check if an AbortSignal is passed if (hasSignal) { - const isNotValidSignal = typeof signal !== 'object' || !('aborted' in signal); + const isNotValidSignal = + typeof signal !== 'object' || !('aborted' in signal); if (isNotValidSignal) return PromiseReject( new ERR_INVALID_ARG_TYPE( @@ -220,7 +221,13 @@ const customPromiseExecFunction = (orig) => { } const promise = new Promise((resolve, reject) => { - if (hasSignal) signal.addEventListener('abort', rejectOnAbortSignal(reject), { once: true }); + if (hasSignal) + signal + .addEventListener( + 'abort', + rejectOnAbortSignal(reject), + { once: true } + ); child = orig(file, options, (err, stdout, stderr) => { if (err !== null) { @@ -232,7 +239,13 @@ const customPromiseExecFunction = (orig) => { } // Let's cleanup once exec is finished - if (hasSignal) signal.removeEventListener('abort', rejectOnAbortSignal(reject), { once: true }); + if (hasSignal) + signal + .removeEventListener( + 'abort', + rejectOnAbortSignal(reject), + { once: true } + ); }); }); @@ -240,11 +253,12 @@ const customPromiseExecFunction = (orig) => { function rejectOnAbortSignal(reject) { return () => { - if (promise && promise.child) promise.child.kill(options.killSignal || 'SIGTERM'); - + if (promise && promise.child) + promise.child.kill(options.killSignal || 'SIGTERM'); + reject(lazyDOMException('AbortError')); }; - }; + } return promise; }; From 3dca212b36a0d632d4c67204b6b9c2bedd4f4f04 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 28 Oct 2020 18:43:46 +0100 Subject: [PATCH 08/25] child_process: check if AbortSignal is aborted before rejecting/resolving --- lib/child_process.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/child_process.js b/lib/child_process.js index b5687fb7c53f16..efb8143c6a0246 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -230,7 +230,9 @@ const customPromiseExecFunction = (orig) => { ); child = orig(file, options, (err, stdout, stderr) => { - if (err !== null) { + if (signal.aborted) { + reject(err); + } else if (err !== null) { err.stdout = stdout; err.stderr = stderr; reject(err); From 4163c449dd63c23df3d8d7028d6fa349c6aa87b7 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 28 Oct 2020 23:56:27 +0100 Subject: [PATCH 09/25] child_process: check for signal before looking is aborted --- lib/child_process.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index efb8143c6a0246..27ef967848809c 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -221,16 +221,16 @@ const customPromiseExecFunction = (orig) => { } const promise = new Promise((resolve, reject) => { - if (hasSignal) - signal - .addEventListener( - 'abort', - rejectOnAbortSignal(reject), - { once: true } - ); + if (hasSignal) { + signal.addEventListener( + 'abort', + rejectOnAbortSignal(reject), + { once: true } + ); + } child = orig(file, options, (err, stdout, stderr) => { - if (signal.aborted) { + if (hasSignal && signal.aborted) { reject(err); } else if (err !== null) { err.stdout = stdout; From 9e3291e06a1053adffab5f738645f663bb7501fd Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Tue, 22 Dec 2020 22:28:07 +0100 Subject: [PATCH 10/25] child_process: refactor signal to use spawn function --- lib/child_process.js | 125 ++++++------------ ...rocess-exec-abortcontroller-promisified.js | 2 - ...ss-execFile-promisified-abortController.js | 63 +++++++++ test/parallel/test-child-process-execfile.js | 29 ++-- 4 files changed, 126 insertions(+), 93 deletions(-) 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 27ef967848809c..dea5a4683c74a2 100644 --- 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 index 3eac82ebfb87d6..6ac9174984c85d 100644 --- 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 new file mode 100644 index 00000000000000..2b2101f4606a1c --- /dev/null +++ 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 index 49a4bdbda29957..aa477653750801 100644 --- 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 From 3b14ebefc41058fb66aa7a9c362ee3c2c1d2fe43 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Tue, 22 Dec 2020 23:16:46 +0100 Subject: [PATCH 11/25] child_process: fix linter issues --- lib/child_process.js | 17 ++++--- ...ss-execFile-promisified-abortController.js | 49 ++++++++++--------- test/parallel/test-child-process-execfile.js | 8 +-- 3 files changed, 40 insertions(+), 34 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index dea5a4683c74a2..2237a1c59c2067 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -42,7 +42,6 @@ const { SafeSet, StringPrototypeSlice, StringPrototypeToUpperCase, - PromiseReject, } = primordials; const { @@ -615,16 +614,21 @@ function spawn(file, args, options) { if (signal.aborted) { onAbortListener(); } else { - signal.addEventListener('abort', onAbortListener, { once: true }); - child.once('close', () => signal.removeEventListener('abort', onAbortListener, { once: true })); + const config = { once: true }; + const remove = () => { + signal.removeEventListener('abort', onAbortListener, config); + }; + + signal.addEventListener('abort', onAbortListener, config); + child.once('close', remove); } 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); @@ -632,13 +636,12 @@ function spawn(file, args, options) { childProcess.emit('error', new AbortError()); }); - } + }; } } debug('spawn', options); child.spawn(options); - return child; } diff --git a/test/parallel/test-child-process-execFile-promisified-abortController.js b/test/parallel/test-child-process-execFile-promisified-abortController.js index 2b2101f4606a1c..ce13376ea2d5b9 100644 --- a/test/parallel/test-child-process-execFile-promisified-abortController.js +++ b/test/parallel/test-child-process-execFile-promisified-abortController.js @@ -4,23 +4,22 @@ 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); +const promisified = 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 }); - + const promise = promisified(process.execPath, [echoFixture, 0], { signal }); + ac.abort(); - assert.rejects(promise, /AbortError/); + promise.catch(common.mustCall((e) => { + assert.strictEqual(e.name, 'AbortError'); + })); } { @@ -29,35 +28,39 @@ const execFilePromisified = promisify(execFile); const { signal } = ac; ac.abort(); - const promise = execFilePromisified(process.execPath, [echoFixture, 0], { signal }); + const promise = promisified(process.execPath, [echoFixture, 0], { signal }); - assert.rejects(promise, /AbortError/); + promise.catch(common.mustCall((e) => { + assert.strictEqual(e.name, '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: {} }); + const signal = {}; + const promise = promisified(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' - }); + promise.catch(common.mustCall((e) => { + assert.strictEqual(e.name, 'TypeError'); + assert.strictEqual(e.code, 'ERR_INVALID_ARG_TYPE'); + assert.strictEqual(e.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!' }); + const signal = 'world!'; + const promise = promisified(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 type string ('world!')` - }); + promise.catch(common.mustCall((e) => { + assert.strictEqual(e.name, 'TypeError'); + assert.strictEqual(e.code, 'ERR_INVALID_ARG_TYPE'); + assert.strictEqual(e.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 index aa477653750801..48ccba97fb8fde 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -71,12 +71,12 @@ const execOpts = { encoding: 'utf8', shell: true }; ac.abort(); try { - execFile(process.execPath, [echoFixture, 0], { signal }); + execFile(process.execPath, [echoFixture, 0], { signal }); } catch (err) { - assert.strictEqual(err.code, 'ABORT_ERR'); - assert.strictEqual(err.name, 'AbortError'); + assert.strictEqual(err.code, 'ABORT_ERR'); + assert.strictEqual(err.name, 'AbortError'); } - + } { From 087f9247efb31c07a2bfcc0eabf73d9197ba653c Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 30 Dec 2020 21:25:06 +0100 Subject: [PATCH 12/25] child_process: revert check for extra callback --- lib/child_process.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 2237a1c59c2067..ddd257ee39a719 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -188,11 +188,6 @@ const customPromiseExecFunction = (orig) => { return (...file) => { let child; - // Check if a callback is passed somehow - if (typeof file[file.length - 1] === 'function') { - file.pop(); - } - const promise = new Promise((resolve, reject) => { child = orig(...file, (err, stdout, stderr) => { if (err !== null) { From 0fd0719fa7279659e778e4168fd8cdc952f9fa68 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 30 Dec 2020 21:39:00 +0100 Subject: [PATCH 13/25] child_process: fix double AbortError emit --- lib/child_process.js | 18 +++++++++++------- ...ess-execFile-promisified-abortController.js | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index ddd257ee39a719..45615d923ee0e5 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -620,10 +620,6 @@ function spawn(file, args, options) { 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); @@ -774,14 +770,22 @@ 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 != null && typeof options === 'object' + ? { ...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-execFile-promisified-abortController.js b/test/parallel/test-child-process-execFile-promisified-abortController.js index ce13376ea2d5b9..69ad1423a46bf8 100644 --- a/test/parallel/test-child-process-execFile-promisified-abortController.js +++ b/test/parallel/test-child-process-execFile-promisified-abortController.js @@ -57,6 +57,21 @@ const promisified = promisify(execFile); const promise = promisified(process.execPath, [echoFixture, 0], { signal }); + promise.catch(common.mustCall((e) => { + assert.strictEqual(e.name, 'TypeError'); + assert.strictEqual(e.code, 'ERR_INVALID_ARG_TYPE'); + assert.strictEqual(e.message, 'The "options.signal" property must be an ' + + "instance of AbortSignal. Received type string ('world!')"); + })); +} + +{ + // Verify that if something different than Abortcontroller.signal + // is passed, ERR_INVALID_ARG_TYPE is thrown + const signal = 'world!'; + const promise = promisified(process.execPath, [echoFixture, 0], { signal }); + + promise.catch(common.mustCall((e) => { assert.strictEqual(e.name, 'TypeError'); assert.strictEqual(e.code, 'ERR_INVALID_ARG_TYPE'); From a41ac9ae04c675771b41a72caefbed9f5518b4a1 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 30 Dec 2020 22:41:48 +0100 Subject: [PATCH 14/25] child_process: fix linter issues --- lib/child_process.js | 15 +++++++-------- test/parallel/test-child-process-execfile.js | 16 +++++++++------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 45615d923ee0e5..0ecb4c116ef5e4 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -770,16 +770,15 @@ 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) { - // Remove signal from options to spawn + // Remove signal from options to spawn // to avoid double emitting of AbortError - const opts = - options != null && typeof options === 'object' - ? { ...options, signal: undefined } - : options; - + const opts = + options != null && typeof options === 'object' ? + { ...options, signal: undefined } : + options; + const child = spawn(file, args, opts); - - + if (options && options.signal) { // Validate signal, if present validateAbortSignal(options.signal, 'options.signal'); diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index 48ccba97fb8fde..e152efaa33056a 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -53,15 +53,18 @@ const execOpts = { encoding: 'utf8', shell: true }; const ac = new AbortController(); const { signal } = ac; - const callback = common.mustCall((err) => { - assert.strictEqual(err.code, 'ABORT_ERR'); - assert.strictEqual(err.name, 'AbortError'); - }); + 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); + }; - execFile(process.execPath, [echoFixture, 0], { signal }, callback); - ac.abort(); // Verify that it still works the same way now that the signal is aborted. test(); + ac.abort(); } { @@ -76,7 +79,6 @@ const execOpts = { encoding: 'utf8', shell: true }; assert.strictEqual(err.code, 'ABORT_ERR'); assert.strictEqual(err.name, 'AbortError'); } - } { From f9af3fdba48a416c55a73c0d5a2bd120861d93b6 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 13 Jan 2021 00:13:05 +0100 Subject: [PATCH 15/25] child_process: fix fork issue with AbortController --- lib/child_process.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 0ecb4c116ef5e4..026ffe16294d9f 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -142,7 +142,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) { @@ -772,12 +772,16 @@ function sanitizeKillSignal(killSignal) { function spawnWithSignal(file, args, options) { // Remove signal from options to spawn // to avoid double emitting of AbortError - const opts = - options != null && typeof options === 'object' ? - { ...options, signal: undefined } : - options; + let child; - const child = spawn(file, args, opts); + if (options && typeof options === 'object' && ('signal' in options)) { + const opts = { ...options }; + delete opts.signal; + + child = spawn(file, args, opts); + } else { + child = spawn(file, args, options); + } if (options && options.signal) { // Validate signal, if present From 2b19cd57bfe4fa09cbe80708d484a7795b9fb607 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 13 Jan 2021 00:13:55 +0100 Subject: [PATCH 16/25] child_process: remove whitespace --- lib/child_process.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 026ffe16294d9f..43fdb374c0fb88 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -49,7 +49,6 @@ const { convertToValidSignal, getSystemErrorName } = require('internal/util'); - const { isArrayBufferView } = require('internal/util/types'); let debug = require('internal/util/debuglog').debuglog( 'child_process', @@ -57,7 +56,6 @@ let debug = require('internal/util/debuglog').debuglog( debug = fn; } ); - const { Buffer } = require('buffer'); const { Pipe, constants: PipeConstants } = internalBinding('pipe_wrap'); From f642015f74295c37ab0ec9a6dc318d50fa74f7fd Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 14 Jan 2021 22:57:01 +0100 Subject: [PATCH 17/25] child_process: re-order alphabetically the options --- lib/child_process.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 43fdb374c0fb88..23758b0063acff 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -259,11 +259,11 @@ function execFile(file /* , args, options, callback */) { 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, - signal: options.signal + windowsVerbatimArguments: !!options.windowsVerbatimArguments }); let encoding; From dade71b5beca95df2e36ee405d88a6c16292b07d Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Mon, 18 Jan 2021 23:08:54 +0100 Subject: [PATCH 18/25] child_process: refactor spawnWithSignal --- lib/child_process.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 23758b0063acff..9fe41bbf886efe 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -770,16 +770,10 @@ function sanitizeKillSignal(killSignal) { function spawnWithSignal(file, args, options) { // Remove signal from options to spawn // to avoid double emitting of AbortError - let child; - - if (options && typeof options === 'object' && ('signal' in options)) { - const opts = { ...options }; - delete opts.signal; - - child = spawn(file, args, opts); - } else { - child = spawn(file, args, options); - } + 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 From 5e7aa7da76b8d965273900f81a3032f79d184b02 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Mon, 18 Jan 2021 23:17:16 +0100 Subject: [PATCH 19/25] child_process: remove wrapper function --- lib/child_process.js | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 9fe41bbf886efe..4db91e961f2078 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -601,8 +601,6 @@ function spawn(file, args, options) { // Validate signal, if present validateAbortSignal(signal, 'options.signal'); - const onAbortListener = onAbortSignal(child); - // Do nothing and throw if already aborted if (signal.aborted) { onAbortListener(); @@ -616,17 +614,15 @@ function spawn(file, args, options) { child.once('close', remove); } - function onAbortSignal(childProcess) { - return function abortListener() { - process.nextTick(() => { - if (childProcess && childProcess.kill) { - childProcess.kill(options.killSignal); - } + function onAbortListener() { + process.nextTick(() => { + if (child?.kill) { + child.kill(options.killSignal); + } - childProcess.emit('error', new AbortError()); - }); - }; - } + child.emit('error', new AbortError()); + }); + }; } debug('spawn', options); From 2305734dde76da1d47d92d7aae9d6b19185063b4 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Mon, 18 Jan 2021 23:49:43 +0100 Subject: [PATCH 20/25] child_process: revert refactor to spawn --- lib/child_process.js | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 4db91e961f2078..0485dd2a2a200a 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -184,21 +184,23 @@ function exec(command, options, callback) { const customPromiseExecFunction = (orig) => { return (...file) => { - let child; - - const promise = new Promise((resolve, reject) => { - child = orig(...file, (err, stdout, stderr) => { - if (err !== null) { - err.stdout = stdout; - err.stderr = stderr; - reject(err); - } else { - resolve({ stdout, stderr }); - } - }); + let resolve; + let reject; + + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; }); - promise.child = child; + promise.child = orig(...file, (err, stdout, stderr) => { + if (err !== null) { + err.stdout = stdout; + err.stderr = stderr; + reject(err); + } else { + resolve({ stdout, stderr }); + } + }); return promise; }; From a60b93599b0894b8ab5f30955676aed353fcdea3 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Mon, 18 Jan 2021 23:54:54 +0100 Subject: [PATCH 21/25] child_process: fix tests --- lib/child_process.js | 2 +- ...rocess-exec-abortcontroller-promisified.js | 23 +++++----- ...ss-execFile-promisified-abortController.js | 43 +++++-------------- 3 files changed, 22 insertions(+), 46 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 0485dd2a2a200a..2f9c944bf4dad0 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -624,7 +624,7 @@ function spawn(file, args, options) { child.emit('error', new AbortError()); }); - }; + } } debug('spawn', options); diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js index 6ac9174984c85d..b207355a9a33aa 100644 --- a/test/parallel/test-child-process-exec-abortcontroller-promisified.js +++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js @@ -6,6 +6,11 @@ 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%'; @@ -25,22 +30,16 @@ if (common.isWindows) { } { - assert.rejects(execPromisifed(pwdcommand, { cwd: dir, signal: {} }), { - code: 'ERR_INVALID_ARG_TYPE', - name: 'TypeError', - message: 'The "options.signal" property must be an ' + - 'instance of AbortSignal. Received an instance of Object' - }); + assert.throws(() => { + execPromisifed(pwdcommand, { cwd: dir, signal: {} }); + }, invalidArgTypeError); } { function signal() {} - assert.rejects(execPromisifed(pwdcommand, { cwd: dir, signal }), { - code: 'ERR_INVALID_ARG_TYPE', - name: 'TypeError', - message: 'The "options.signal" property must be an ' + - 'instance of AbortSignal. Received function signal' - }); + assert.throws(function() { + execPromisifed(pwdcommand, { cwd: dir, signal }); + }, invalidArgTypeError); } { diff --git a/test/parallel/test-child-process-execFile-promisified-abortController.js b/test/parallel/test-child-process-execFile-promisified-abortController.js index 69ad1423a46bf8..d7c34c11a09a2d 100644 --- a/test/parallel/test-child-process-execFile-promisified-abortController.js +++ b/test/parallel/test-child-process-execFile-promisified-abortController.js @@ -8,6 +8,10 @@ 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 @@ -39,43 +43,16 @@ const promisified = promisify(execFile); // Verify that if something different than Abortcontroller.signal // is passed, ERR_INVALID_ARG_TYPE is thrown const signal = {}; - const promise = promisified(process.execPath, [echoFixture, 0], { signal }); - - promise.catch(common.mustCall((e) => { - assert.strictEqual(e.name, 'TypeError'); - assert.strictEqual(e.code, 'ERR_INVALID_ARG_TYPE'); - assert.strictEqual(e.message, 'The "options.signal" property must be an ' + - 'instance of AbortSignal. Received an instance of Object'); - })); - + 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!'; - const promise = promisified(process.execPath, [echoFixture, 0], { signal }); - - - promise.catch(common.mustCall((e) => { - assert.strictEqual(e.name, 'TypeError'); - assert.strictEqual(e.code, 'ERR_INVALID_ARG_TYPE'); - assert.strictEqual(e.message, 'The "options.signal" property must be an ' + - "instance of AbortSignal. Received type string ('world!')"); - })); -} - -{ - // Verify that if something different than Abortcontroller.signal - // is passed, ERR_INVALID_ARG_TYPE is thrown - const signal = 'world!'; - const promise = promisified(process.execPath, [echoFixture, 0], { signal }); - - - promise.catch(common.mustCall((e) => { - assert.strictEqual(e.name, 'TypeError'); - assert.strictEqual(e.code, 'ERR_INVALID_ARG_TYPE'); - assert.strictEqual(e.message, 'The "options.signal" property must be an ' + - "instance of AbortSignal. Received type string ('world!')"); - })); + assert.throws(() => { + promisified(process.execPath, [echoFixture, 0], { signal }); + }, invalidArgTypeError); } From fe077b0d89ec60408d96896d77f24f0bea536cc8 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 21 Jan 2021 23:03:06 +0100 Subject: [PATCH 22/25] child_process: refactor --- lib/child_process.js | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 2f9c944bf4dad0..b8412c73a88025 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -183,16 +183,15 @@ function exec(command, options, callback) { } const customPromiseExecFunction = (orig) => { - return (...file) => { + return (...args) => { let resolve; let reject; - const promise = new Promise((res, rej) => { resolve = res; reject = rej; }); - promise.child = orig(...file, (err, stdout, stderr) => { + promise.child = orig(...args, (err, stdout, stderr) => { if (err !== null) { err.stdout = stdout; err.stderr = stderr; @@ -608,19 +607,18 @@ function spawn(file, args, options) { onAbortListener(); } else { const config = { once: true }; - const remove = () => { - signal.removeEventListener('abort', onAbortListener, config); - }; signal.addEventListener('abort', onAbortListener, config); - child.once('close', remove); + child.once('close', + () => + signal + .removeEventListener('abort', onAbortListener, config) + ); } function onAbortListener() { process.nextTick(() => { - if (child?.kill) { - child.kill(options.killSignal); - } + child?.kill?.(options.killSignal); child.emit('error', new AbortError()); }); From 72ff68ac34b50a2d175fc967f875d8172eab1475 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 21 Jan 2021 23:15:28 +0100 Subject: [PATCH 23/25] child_process: refactor tests --- ...d-process-exec-abortcontroller-promisified.js | 6 +++--- ...ocess-execFile-promisified-abortController.js | 16 ++++++++-------- test/parallel/test-child-process-execfile.js | 8 ++++---- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js index b207355a9a33aa..fa00f2d2177635 100644 --- a/test/parallel/test-child-process-exec-abortcontroller-promisified.js +++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js @@ -25,7 +25,7 @@ if (common.isWindows) { const ac = new AbortController(); const signal = ac.signal; const promise = execPromisifed(pwdcommand, { cwd: dir, signal }); - assert.rejects(promise, /AbortError/); + assert.rejects(promise, /AbortError/).then(common.mustCall()); ac.abort(); } @@ -37,7 +37,7 @@ if (common.isWindows) { { function signal() {} - assert.throws(function() { + assert.throws(() => { execPromisifed(pwdcommand, { cwd: dir, signal }); }, invalidArgTypeError); } @@ -47,5 +47,5 @@ if (common.isWindows) { const signal = (ac.abort(), ac.signal); const promise = execPromisifed(pwdcommand, { cwd: dir, signal }); - assert.rejects(promise, /AbortError/); + 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 index d7c34c11a09a2d..ccfcafc5d51e58 100644 --- a/test/parallel/test-child-process-execFile-promisified-abortController.js +++ b/test/parallel/test-child-process-execFile-promisified-abortController.js @@ -21,9 +21,10 @@ const invalidArgTypeError = { ac.abort(); - promise.catch(common.mustCall((e) => { - assert.strictEqual(e.name, 'AbortError'); - })); + assert.rejects( + promise, + { name: 'AbortError' } + ).then(common.mustCall()); } { @@ -32,11 +33,10 @@ const invalidArgTypeError = { const { signal } = ac; ac.abort(); - const promise = promisified(process.execPath, [echoFixture, 0], { signal }); - - promise.catch(common.mustCall((e) => { - assert.strictEqual(e.name, 'AbortError'); - })); + assert.rejects( + promisified(process.execPath, [echoFixture, 0], { signal }), + { name: 'AbortError' } + ).then(common.mustCall()); } { diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index e152efaa33056a..a8f0cabacc2860 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -73,12 +73,12 @@ const execOpts = { encoding: 'utf8', shell: true }; const { signal } = ac; ac.abort(); - try { - execFile(process.execPath, [echoFixture, 0], { signal }); - } catch (err) { + 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); } { From 026021afe4822ab1bbe0c0a1f6f39ce561f7ef81 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 21 Jan 2021 23:46:17 +0100 Subject: [PATCH 24/25] child_process: refactor listeners args --- lib/child_process.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index b8412c73a88025..39f6e2d3fca758 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -606,13 +606,11 @@ function spawn(file, args, options) { if (signal.aborted) { onAbortListener(); } else { - const config = { once: true }; - - signal.addEventListener('abort', onAbortListener, config); + signal.addEventListener('abort', onAbortListener, { once: true }); child.once('close', () => signal - .removeEventListener('abort', onAbortListener, config) + .removeEventListener('abort', onAbortListener) ); } From c0930354228f1e14be1c1a5a1e400c913e7f3970 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 22 Jan 2021 00:07:24 +0100 Subject: [PATCH 25/25] child_process: inline listener --- lib/child_process.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 39f6e2d3fca758..30ff49f5f30267 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -608,10 +608,7 @@ function spawn(file, args, options) { } else { signal.addEventListener('abort', onAbortListener, { once: true }); child.once('close', - () => - signal - .removeEventListener('abort', onAbortListener) - ); + () => signal.removeEventListener('abort', onAbortListener)); } function onAbortListener() {