Skip to content
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

Make the spawned process cancelable #189

Merged
merged 19 commits into from Mar 19, 2019
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 23 additions & 6 deletions index.js
Expand Up @@ -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);

Expand All @@ -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;
Expand All @@ -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;
Expand 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}`;
}
Expand Down Expand Up @@ -239,6 +244,7 @@ module.exports = (command, args, options) => {

let timeoutId = null;
let timedOut = false;
let canceled = false;

const cleanup = () => {
if (timeoutId) {
Expand Down Expand Up @@ -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
Expand All @@ -334,7 +341,8 @@ module.exports = (command, args, options) => {
killed: false,
signal: null,
cmd: joinedCommand,
timedOut: false
timedOut: false,
canceled: false
Copy link
Owner

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 with killed, but I want to rename that one at some point too. Use the isCanceled naming for all the canceled variables.

};
}), destroy);

Expand All @@ -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 () {
Copy link
Owner

Choose a reason for hiding this comment

The 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) {
Expand Down
16 changes: 16 additions & 0 deletions readme.md
Expand Up @@ -52,6 +52,15 @@ const execa = require('execa');
const {stdout} = await execa.shell('echo unicorns');
//=> 'unicorns'

// Cancelling a spawned process
const spawned = execa("node");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single-quotes

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think process would be a better name than spawned.

Copy link
Collaborator

@ehmicky ehmicky Mar 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this confuse readers about the global variable named process? Child processes returned by spawn() and global process have different methods/properties (although some are shared).

Node API doc calls it subprocess: https://nodejs.org/api/child_process.html#child_process_options_detached

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, subprocess is better.

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 {
Expand Down Expand Up @@ -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()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear enough what spawned is. And it's weird to place the docs here. I think it would be better to document it in the execa() section.


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`
Expand Down
54 changes: 54 additions & 0 deletions test.js
Expand Up @@ -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');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const spawned = execa('noop');
const subprocess = execa('noop');

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);
});