From 5f5f5b77a3f447ef6810b44ab41d08eb03e524f7 Mon Sep 17 00:00:00 2001 From: ehmicky Date: Sun, 26 May 2024 10:18:12 +0100 Subject: [PATCH] Improve `cancelSignal` option --- lib/arguments/options.js | 2 + lib/methods/main-async.js | 8 +-- lib/resolve/exit-async.js | 2 +- lib/resolve/wait-subprocess.js | 3 + lib/terminate/cancel.js | 26 +++++++++ test/helpers/early-error.js | 2 +- test/return/final-error.js | 13 +++-- test/terminate/cancel.js | 103 ++++++++++++++++++++++++++++++++- test/terminate/kill-signal.js | 1 + 9 files changed, 146 insertions(+), 14 deletions(-) create mode 100644 lib/terminate/cancel.js diff --git a/lib/arguments/options.js b/lib/arguments/options.js index 84ad416bdc..ff77809729 100644 --- a/lib/arguments/options.js +++ b/lib/arguments/options.js @@ -4,6 +4,7 @@ import crossSpawn from 'cross-spawn'; import {npmRunPathEnv} from 'npm-run-path'; import {normalizeForceKillAfterDelay} from '../terminate/kill.js'; import {normalizeKillSignal} from '../terminate/signal.js'; +import {validateCancelSignal} from '../terminate/cancel.js'; import {validateTimeout} from '../terminate/timeout.js'; import {handleNodeOption} from '../methods/node.js'; import {validateIpcInputOption} from '../ipc/ipc-input.js'; @@ -25,6 +26,7 @@ export const normalizeOptions = (filePath, rawArguments, rawOptions) => { validateTimeout(options); validateEncoding(options); validateIpcInputOption(options); + validateCancelSignal(options); options.shell = normalizeFileUrl(options.shell); options.env = getEnv(options); options.killSignal = normalizeKillSignal(options.killSignal); diff --git a/lib/methods/main-async.js b/lib/methods/main-async.js index 51eb572674..4c6a6b191b 100644 --- a/lib/methods/main-async.js +++ b/lib/methods/main-async.js @@ -71,12 +71,12 @@ const handleAsyncArguments = (rawFile, rawArguments, rawOptions) => { // Options normalization logic specific to async methods. // Prevent passing the `timeout` option directly to `child_process.spawn()`. -const handleAsyncOptions = ({timeout, signal, cancelSignal, ...options}) => { +const handleAsyncOptions = ({timeout, signal, ...options}) => { if (signal !== undefined) { throw new TypeError('The "signal" option has been renamed to "cancelSignal" instead.'); } - return {...options, timeoutDuration: timeout, signal: cancelSignal}; + return {...options, timeoutDuration: timeout}; }; const spawnSubprocessAsync = ({file, commandArguments, options, startTime, verboseInfo, command, escapedCommand, fileDescriptors}) => { @@ -130,7 +130,7 @@ const spawnSubprocessAsync = ({file, commandArguments, options, startTime, verbo // Asynchronous logic, as opposed to the previous logic which can be run synchronously, i.e. can be returned to user right away const handlePromise = async ({subprocess, options, startTime, verboseInfo, fileDescriptors, originalStreams, command, escapedCommand, onInternalError, controller}) => { - const context = {timedOut: false}; + const context = {timedOut: false, isCanceled: false}; const [ errorInfo, @@ -175,7 +175,7 @@ const getAsyncResult = ({errorInfo, exitCode, signal, stdio, all, ipcOutput, con command, escapedCommand, timedOut: context.timedOut, - isCanceled: options.signal?.aborted === true, + isCanceled: context.isCanceled, isMaxBuffer: errorInfo.error instanceof MaxBufferError, exitCode, signal, diff --git a/lib/resolve/exit-async.js b/lib/resolve/exit-async.js index 3b0cfc740c..12cda70fed 100644 --- a/lib/resolve/exit-async.js +++ b/lib/resolve/exit-async.js @@ -2,7 +2,7 @@ import {once} from 'node:events'; import {DiscardedError} from '../return/final-error.js'; // If `error` is emitted before `spawn`, `exit` will never be emitted. -// However, `error` might be emitted after `spawn`, e.g. with the `cancelSignal` option. +// However, `error` might be emitted after `spawn`. // In that case, `exit` will still be emitted. // Since the `exit` event contains the signal name, we want to make sure we are listening for it. // This function also takes into account the following unlikely cases: diff --git a/lib/resolve/wait-subprocess.js b/lib/resolve/wait-subprocess.js index 12a274e4e5..35f57ee95c 100644 --- a/lib/resolve/wait-subprocess.js +++ b/lib/resolve/wait-subprocess.js @@ -1,6 +1,7 @@ import {once} from 'node:events'; import {isStream as isNodeStream} from 'is-stream'; import {throwOnTimeout} from '../terminate/timeout.js'; +import {throwOnCancel} from '../terminate/cancel.js'; import {isStandardStream} from '../utils/standard-stream.js'; import {TRANSFORM_TYPES} from '../stdio/type.js'; import {getBufferedData} from '../io/contents.js'; @@ -20,6 +21,7 @@ export const waitForSubprocessResult = async ({ maxBuffer, lines, timeoutDuration: timeout, + cancelSignal, stripFinalNewline, ipc, ipcInput, @@ -87,6 +89,7 @@ export const waitForSubprocessResult = async ({ onInternalError, throwOnSubprocessError(subprocess, controller), ...throwOnTimeout(subprocess, timeout, context, controller), + ...throwOnCancel(subprocess, cancelSignal, context, controller), ]); } catch (error) { return Promise.all([ diff --git a/lib/terminate/cancel.js b/lib/terminate/cancel.js new file mode 100644 index 0000000000..11dfffd16f --- /dev/null +++ b/lib/terminate/cancel.js @@ -0,0 +1,26 @@ +import {once} from 'node:events'; + +// Validate the `cancelSignal` option +export const validateCancelSignal = ({cancelSignal}) => { + if (cancelSignal !== undefined && Object.prototype.toString.call(cancelSignal) !== '[object AbortSignal]') { + throw new Error(`The \`cancelSignal\` option must be an AbortSignal: ${String(cancelSignal)}`); + } +}; + +// Terminate the subprocess when aborting the `cancelSignal` option +export const throwOnCancel = (subprocess, cancelSignal, context, controller) => cancelSignal === undefined + ? [] + : [terminateOnCancel(subprocess, cancelSignal, context, controller)]; + +const terminateOnCancel = async (subprocess, cancelSignal, context, {signal}) => { + await onAbortedSignal(cancelSignal, signal); + context.isCanceled = true; + subprocess.kill(); + throw cancelSignal.reason; +}; + +const onAbortedSignal = async (cancelSignal, signal) => { + if (!cancelSignal.aborted) { + await once(cancelSignal, 'abort', {signal}); + } +}; diff --git a/test/helpers/early-error.js b/test/helpers/early-error.js index aca36c18df..ced3752cc0 100644 --- a/test/helpers/early-error.js +++ b/test/helpers/early-error.js @@ -1,6 +1,6 @@ import {execa, execaSync} from '../../index.js'; -export const earlyErrorOptions = {cancelSignal: false}; +export const earlyErrorOptions = {detached: 'true'}; export const getEarlyErrorSubprocess = options => execa('empty.js', {...earlyErrorOptions, ...options}); export const earlyErrorOptionsSync = {maxBuffer: false}; export const getEarlyErrorSubprocessSync = options => execaSync('empty.js', {...earlyErrorOptionsSync, ...options}); diff --git a/test/return/final-error.js b/test/return/final-error.js index c0ce119bf4..f622089c3c 100644 --- a/test/return/final-error.js +++ b/test/return/final-error.js @@ -99,12 +99,13 @@ test('error.cause is set on error event', async t => { test('error.cause is set if error.isCanceled', async t => { const controller = new AbortController(); const subprocess = execa('forever.js', {cancelSignal: controller.signal}); - const error = new Error('test'); - controller.abort(error); - const {isCanceled, isTerminated, cause} = await t.throwsAsync(subprocess); - t.true(isCanceled); - t.false(isTerminated); - t.is(cause.cause, error); + const cause = new Error('test'); + controller.abort(cause); + const error = await t.throwsAsync(subprocess); + t.true(error.isCanceled); + t.true(error.isTerminated); + t.is(error.signal, 'SIGTERM'); + t.is(error.cause, cause); }); test('error.cause is not set if error.isTerminated with .kill(error)', async t => { diff --git a/test/terminate/cancel.js b/test/terminate/cancel.js index 8e51118adb..adb6816ce1 100644 --- a/test/terminate/cancel.js +++ b/test/terminate/cancel.js @@ -1,10 +1,22 @@ -import {once} from 'node:events'; +import {once, getEventListeners} from 'node:events'; import test from 'ava'; import {execa, execaSync} from '../../index.js'; import {setFixtureDirectory} from '../helpers/fixtures-directory.js'; +import {foobarString} from '../helpers/input.js'; setFixtureDirectory(); +const testValidCancelSignal = (t, cancelSignal) => { + t.throws(() => { + execa('empty.js', {cancelSignal}); + }, {message: /must be an AbortSignal/}); +}; + +test('cancelSignal option cannot be AbortController', testValidCancelSignal, new AbortController()); +test('cancelSignal option cannot be {}', testValidCancelSignal, {}); +test('cancelSignal option cannot be null', testValidCancelSignal, null); +test('cancelSignal option cannot be a symbol', testValidCancelSignal, Symbol('test')); + test('result.isCanceled is false when abort isn\'t called (success)', async t => { const {isCanceled} = await execa('noop.js'); t.false(isCanceled); @@ -48,9 +60,59 @@ test('calling abort is considered a signal termination', async t => { const subprocess = execa('forever.js', {cancelSignal: abortController.signal}); await once(subprocess, 'spawn'); abortController.abort(); - const {isTerminated, signal} = await t.throwsAsync(subprocess); + const {isCanceled, isTerminated, signal} = await t.throwsAsync(subprocess); + t.true(isCanceled); + t.true(isTerminated); + t.is(signal, 'SIGTERM'); +}); + +test('cancelSignal can already be aborted', async t => { + const cancelSignal = AbortSignal.abort(); + const {isCanceled, isTerminated, signal} = await t.throwsAsync(execa('forever.js', {cancelSignal})); + t.true(isCanceled); t.true(isTerminated); t.is(signal, 'SIGTERM'); + t.deepEqual(getEventListeners(cancelSignal, 'abort'), []); +}); + +test('calling abort does not emit the "error" event', async t => { + const abortController = new AbortController(); + const subprocess = execa('forever.js', {cancelSignal: abortController.signal}); + let error; + subprocess.once('error', errorArgument => { + error = errorArgument; + }); + abortController.abort(); + const {isCanceled} = await t.throwsAsync(subprocess); + t.true(isCanceled); + t.is(error, undefined); +}); + +test('calling abort cleans up listeners on cancelSignal, called', async t => { + const abortController = new AbortController(); + const subprocess = execa('forever.js', {cancelSignal: abortController.signal}); + t.is(getEventListeners(abortController.signal, 'abort').length, 1); + abortController.abort(); + const {isCanceled} = await t.throwsAsync(subprocess); + t.true(isCanceled); + t.is(getEventListeners(abortController.signal, 'abort').length, 0); +}); + +test('calling abort cleans up listeners on cancelSignal, not called', async t => { + const abortController = new AbortController(); + const subprocess = execa('noop.js', {cancelSignal: abortController.signal}); + t.is(getEventListeners(abortController.signal, 'abort').length, 1); + await subprocess; + t.is(getEventListeners(abortController.signal, 'abort').length, 0); +}); + +test('calling abort cleans up listeners on cancelSignal, already aborted', async t => { + const cancelSignal = AbortSignal.abort(); + const subprocess = execa('noop.js', {cancelSignal}); + t.is(getEventListeners(cancelSignal, 'abort').length, 0); + const {isCanceled} = await t.throwsAsync(subprocess); + t.true(isCanceled); + t.is(getEventListeners(cancelSignal, 'abort').length, 0); }); test('calling abort throws an error with message "Command was canceled"', async t => { @@ -60,6 +122,43 @@ test('calling abort throws an error with message "Command was canceled"', async await t.throwsAsync(subprocess, {message: /Command was canceled/}); }); +test('calling abort with no argument keeps error properties', async t => { + const abortController = new AbortController(); + const subprocess = execa('noop.js', {cancelSignal: abortController.signal}); + abortController.abort(); + const {cause, originalMessage, shortMessage, message} = await t.throwsAsync(subprocess); + t.is(cause.message, 'This operation was aborted'); + t.is(cause.name, 'AbortError'); + t.is(originalMessage, 'This operation was aborted'); + t.is(shortMessage, 'Command was canceled: noop.js\nThis operation was aborted'); + t.is(message, 'Command was canceled: noop.js\nThis operation was aborted'); +}); + +test('calling abort with an error instance keeps error properties', async t => { + const abortController = new AbortController(); + const subprocess = execa('noop.js', {cancelSignal: abortController.signal}); + const error = new Error(foobarString); + error.code = foobarString; + abortController.abort(error); + const {cause, originalMessage, shortMessage, message, code} = await t.throwsAsync(subprocess); + t.is(cause, error); + t.is(originalMessage, foobarString); + t.is(shortMessage, `Command was canceled: noop.js\n${foobarString}`); + t.is(message, `Command was canceled: noop.js\n${foobarString}`); + t.is(code, foobarString); +}); + +test('calling abort with null keeps error properties', async t => { + const abortController = new AbortController(); + const subprocess = execa('noop.js', {cancelSignal: abortController.signal}); + abortController.abort(null); + const {cause, originalMessage, shortMessage, message} = await t.throwsAsync(subprocess); + t.is(cause, null); + t.is(originalMessage, 'null'); + t.is(shortMessage, 'Command was canceled: noop.js\nnull'); + t.is(message, 'Command was canceled: noop.js\nnull'); +}); + test('calling abort twice should show the same behaviour as calling it once', async t => { const abortController = new AbortController(); const subprocess = execa('noop.js', {cancelSignal: abortController.signal}); diff --git a/test/terminate/kill-signal.js b/test/terminate/kill-signal.js index 273a4f22a7..c3647d6dd6 100644 --- a/test/terminate/kill-signal.js +++ b/test/terminate/kill-signal.js @@ -134,6 +134,7 @@ test('subprocess double errors are handled after spawn', async t => { subprocess.emit('error', cause); await setImmediate(); abortController.abort(); + subprocess.emit('error', cause); const error = await t.throwsAsync(subprocess); t.is(error.cause, cause); t.is(error.exitCode, undefined);