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 12 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
26 changes: 20 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,12 @@ 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) {
ehmicky marked this conversation as resolved.
Show resolved Hide resolved
canceled = true;
spawned.kill();
}
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
6 changes: 6 additions & 0 deletions readme.md
Expand Up @@ -52,6 +52,12 @@ const execa = require('execa');
const {stdout} = await execa.shell('echo unicorns');
//=> 'unicorns'

// Cancelling a spawned process
const spawned = execa("echo unicorns");
spawned.catch(error => {
console.log(error.message);
}); // To handle the CancelError
ammarbinfaisal1 marked this conversation as resolved.
Show resolved Hide resolved
spawned.cancel();
ehmicky marked this conversation as resolved.
Show resolved Hide resolved

// Catching an error
try {
Expand Down
39 changes: 39 additions & 0 deletions test.js
Expand Up @@ -586,3 +586,42 @@ 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);
ehmicky marked this conversation as resolved.
Show resolved Hide resolved
});

test('result.canceled is false when spawned.cancel isn\'t called', async t => {
const spawned = execa('noop');
const result = await spawned;
ehmicky marked this conversation as resolved.
Show resolved Hide resolved
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();
const error = await t.throwsAsync(async () => {
await spawned;
});
t.regex(error.message, /Command\swas\scanceled/);
ehmicky marked this conversation as resolved.
Show resolved Hide resolved
});
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(async () => {
await 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(async () => {
await spawned;
});
t.false(error.canceled);
});