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 18 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, isCanceled, 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, isCanceled});
Copy link
Owner

Choose a reason for hiding this comment

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

Don't do unrelated changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might have done this while fixing some merge conflict; that's the only time I touched those two variables exitCode and exitCodeName

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.isCanceled = isCanceled;

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, isCanceled}) {
if (timedOut) {
return `timed out after ${timeout} milliseconds`;
}

if (isCanceled) {
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 isCanceled = 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 || isCanceled) {
const error = makeError(result, {
joinedCommand,
parsed,
timedOut
timedOut,
isCanceled
});

// 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,
isCanceled: false
};
}), 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 = () => {
if (spawned.killed) {
return;
}

if (spawned.kill()) {
isCanceled = 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
12 changes: 12 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 subprocess = execa('node');
setTimeout(() => { spawned.cancel() }, 1000);
try {
await subprocess;
} catch (error) {
console.log(subprocess.killed); // true
console.log(error.isCanceled); // true
}

// Catching an error
try {
Expand Down Expand Up @@ -112,6 +121,9 @@ Returns a [`child_process` instance](https://nodejs.org/api/child_process.html#c

It exposes an additional `.all` stream, with `stdout` and `stderr` interleaved.

It can be canceled with `.cancel` method which throws an error with `error.isCanceled` equal to `true`, provided that the process gets canceled.
Process would not get canceled if it has already exited.

The promise result is an `Object` with `stdout`, `stderr` and `all` properties.

### execa.stdout(file, [arguments], [options])
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.isCanceled is false when spawned.cancel isn\'t called', async t => {
const result = await execa('noop');
t.false(result.isCanceled);
});

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

test('error.isCanceled is false when kill method is used', async t => {
const spawned = execa('noop');
spawned.kill();
const error = await t.throwsAsync(spawned);
t.false(error.isCanceled);
});

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

test('calling cancel method on a process which has been killed does not make error.isCanceled true', async t => {
const spawned = execa('noop');
spawned.kill();
const error = await t.throwsAsync(spawned);
t.false(error.isCanceled);
});