-
-
Notifications
You must be signed in to change notification settings - Fork 236
Make the spawned process cancelable #189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 15 commits
3b247c5
5a74c26
88beb24
7c743e0
71b6abf
cef4609
6c1e31c
7786ebd
bef739a
a24a895
629ba5f
9cd06d4
282124c
2c70e6c
3cb85f1
089c7b0
d85eb02
8cd3fcf
f5c35c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,7 +143,7 @@ function getStream(process, stream, {encoding, buffer, maxBuffer}) { | |
function makeError(result, options) { | ||
const {stdout, stderr, code, signal} = result; | ||
let {error} = result; | ||
const {joinedCommand, timedOut, parsed: {options: {timeout}}} = options; | ||
const {joinedCommand, timedOut, canceled, parsed: {options: {timeout}}} = options; | ||
|
||
const [exitCodeName, exitCode] = getCode(result, code); | ||
|
||
|
@@ -152,7 +152,7 @@ function makeError(result, options) { | |
error = new Error(message); | ||
} | ||
|
||
const prefix = getErrorPrefix({timedOut, timeout, signal, exitCodeName, exitCode}); | ||
const prefix = getErrorPrefix({timedOut, timeout, signal, exitCode, exitCodeName, canceled}); | ||
error.message = `Command ${prefix}: ${error.message}`; | ||
|
||
error.code = exitCode || exitCodeName; | ||
|
@@ -164,6 +164,7 @@ function makeError(result, options) { | |
error.signal = signal || null; | ||
error.cmd = joinedCommand; | ||
error.timedOut = Boolean(timedOut); | ||
error.canceled = canceled; | ||
|
||
if ('all' in result) { | ||
error.all = result.all; | ||
|
@@ -184,11 +185,15 @@ function getCode({error = {}}, code) { | |
return []; | ||
} | ||
|
||
function getErrorPrefix({timedOut, timeout, signal, exitCodeName, exitCode}) { | ||
function getErrorPrefix({timedOut, timeout, signal, exitCodeName, exitCode, canceled}) { | ||
if (timedOut) { | ||
return `timed out after ${timeout} milliseconds`; | ||
} | ||
|
||
if (canceled) { | ||
return 'was canceled'; | ||
} | ||
|
||
if (signal) { | ||
return `was killed with ${signal}`; | ||
} | ||
|
@@ -239,6 +244,7 @@ module.exports = (command, args, options) => { | |
|
||
let timeoutId = null; | ||
let timedOut = false; | ||
let canceled = false; | ||
|
||
const cleanup = () => { | ||
if (timeoutId) { | ||
|
@@ -304,11 +310,12 @@ module.exports = (command, args, options) => { | |
result.stderr = results[2]; | ||
result.all = results[3]; | ||
|
||
if (result.error || result.code !== 0 || result.signal !== null) { | ||
if (result.error || result.code !== 0 || result.signal !== null || canceled) { | ||
const error = makeError(result, { | ||
joinedCommand, | ||
parsed, | ||
timedOut | ||
timedOut, | ||
canceled | ||
}); | ||
|
||
// TODO: missing some timeout logic for killed | ||
|
@@ -334,7 +341,8 @@ module.exports = (command, args, options) => { | |
killed: false, | ||
signal: null, | ||
cmd: joinedCommand, | ||
timedOut: false | ||
timedOut: false, | ||
canceled: false | ||
}; | ||
}), destroy); | ||
|
||
|
@@ -347,6 +355,15 @@ module.exports = (command, args, options) => { | |
// eslint-disable-next-line promise/prefer-await-to-then | ||
spawned.then = (onFulfilled, onRejected) => handlePromise().then(onFulfilled, onRejected); | ||
spawned.catch = onRejected => handlePromise().catch(onRejected); | ||
spawned.cancel = function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use arrow function |
||
if (spawned.killed) { | ||
return; | ||
} | ||
|
||
if (spawned.kill()) { | ||
canceled = true; | ||
} | ||
ammarbinfaisal1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
// TOOD: Remove the `if`-guard when targeting Node.js 10 | ||
if (Promise.prototype.finally) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,15 @@ const execa = require('execa'); | |
const {stdout} = await execa.shell('echo unicorns'); | ||
//=> 'unicorns' | ||
|
||
// Cancelling a spawned process | ||
const spawned = execa("node"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Single-quotes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this confuse readers about the global variable named Node API doc calls it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah, |
||
spawned.cancel(); | ||
ehmicky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
try { | ||
await spawned; | ||
} catch (error) { | ||
console.log(spawned.killed); // true | ||
console.log(error.canceled); // true | ||
} | ||
|
||
// Catching an error | ||
try { | ||
|
@@ -146,6 +155,13 @@ Execute a command synchronously through the system shell. | |
|
||
Returns the same result object as [`child_process.spawnSync`](https://nodejs.org/api/child_process.html#child_process_child_process_spawnsync_command_args_options). | ||
|
||
### spawned.cancel() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not clear enough what |
||
|
||
Cancel a process spawned using execa. Calling this method kills it. | ||
|
||
Throws an error with `error.canceled` equal to `true`, provided that the process gets canceled. | ||
Process would not get canceled if it has already exited or has been killed by `spawned.kill()`. | ||
ehmicky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
### options | ||
|
||
Type: `Object` | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -586,3 +586,57 @@ if (Promise.prototype.finally) { | |||||
t.is(result.message, 'called'); | ||||||
}); | ||||||
} | ||||||
|
||||||
test('cancel method kills the spawned process', t => { | ||||||
const spawned = execa('node'); | ||||||
spawned.cancel(); | ||||||
t.true(spawned.killed); | ||||||
}); | ||||||
|
||||||
test('result.canceled is false when spawned.cancel isn\'t called', async t => { | ||||||
const result = await execa('noop'); | ||||||
t.false(result.canceled); | ||||||
}); | ||||||
|
||||||
test('calling cancel method throws an error with message "Command was canceled"', async t => { | ||||||
const spawned = execa('noop'); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
spawned.cancel(); | ||||||
await t.throwsAsync(spawned, {message: /Command was canceled/}); | ||||||
}); | ||||||
ehmicky marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
test('error.canceled is true when cancel method is used', async t => { | ||||||
const spawned = execa('noop'); | ||||||
spawned.cancel(); | ||||||
const error = await t.throwsAsync(spawned); | ||||||
ehmicky marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
t.true(error.canceled); | ||||||
}); | ||||||
|
||||||
test('error.canceled is false when kill method is used', async t => { | ||||||
ehmicky marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
const spawned = execa('noop'); | ||||||
spawned.kill(); | ||||||
const error = await t.throwsAsync(spawned); | ||||||
t.false(error.canceled); | ||||||
}); | ||||||
|
||||||
test('calling cancel method twice should show the same behaviour as calling it once', async t => { | ||||||
const spawned = execa('noop'); | ||||||
spawned.cancel(); | ||||||
spawned.cancel(); | ||||||
const error = await t.throwsAsync(spawned); | ||||||
t.true(error.canceled); | ||||||
t.true(spawned.killed); | ||||||
ehmicky marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
}); | ||||||
|
||||||
test('calling cancel method on a successfuly completed process does not make result.cancel true', async t => { | ||||||
const spawned = execa('noop'); | ||||||
const result = await spawned; | ||||||
spawned.cancel(); | ||||||
t.false(result.canceled); | ||||||
}); | ||||||
|
||||||
test('calling cancel method on a process which has been killed does not make error.canceled true', async t => { | ||||||
const spawned = execa('noop'); | ||||||
spawned.kill(); | ||||||
const error = await t.throwsAsync(spawned); | ||||||
t.false(error.canceled); | ||||||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to name it
isCanceled
. I realize it's inconsistent withkilled
, but I want to rename that one at some point too. Use theisCanceled
naming for all thecanceled
variables.