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.

Sign up for GitHub

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

Jump to bottom

Fix timeout option not leaving early when on sleep mode #199

Merged
merged 4 commits into from Apr 12, 2019
Merged

Fix timeout option not leaving early when on sleep mode #199

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a97e8fb
Fix timeout option not leaving early when on `sleep` mode
Apr 3, 2019
aee46e7
Extends expected timeout margin
Apr 3, 2019
582f2b0
Run time depend tests serially
Apr 3, 2019
fd6bec8
Extract final step to function to remove lint warning
Apr 12, 2019
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
4 changes: 4 additions & 0 deletions fixtures/sleeper
View file
Open in desktop
@@ -0,0 +1,4 @@
#!/bin/bash

sleep 5
echo "ok"
111 changes: 69 additions & 42 deletions index.js
View file
Open in desktop
Expand Up @@ -267,9 +267,25 @@ const execa = (command, args, options) => {
}, parsed.options.timeout);
}

const resolvable = (() => {
Copy link
Owner

Choose a reason for hiding this comment

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

This is better known as a deferred, and is usually considered a bad practice: https://github.com/petkaantonov/bluebird/wiki/Promise-Anti-patterns#the-deferred-anti-pattern

I haven't looked closely into the changes here, but maybe there's a way to solve it without using a deferred? If not, that's fine too. Sometimes it's needed. But usually when someone grabs for a deferred, there's usually a better way.

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 think we fall into the "it's needed" case.

You might have to use a deferred object when wrapping a callback API that doesn't follow the standard convention. Like setTimeout.

The goal is to resolve as soon as the process complete or the setTimeout ends. I could use some kind of cancellation, but I don't see any better solution right now.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I don't see a better way either. I was just trying my luck in case you could think of a better way.

let extracted;
const promise = new Promise(resolve => {
extracted = resolve;
});
promise.resolve = extracted;
return promise;
})();

const processDone = new Promise(resolve => {
spawned.on('exit', (code, signal) => {
cleanup();

if (timedOut) {
resolvable.resolve([
{code, signal}, '', '', ''
]);
}

resolve({code, signal});
ehmicky marked this conversation as resolved.
Show resolved Hide resolved
});

Expand Down Expand Up @@ -301,51 +317,62 @@ const execa = (command, args, options) => {
}

// TODO: Use native "finally" syntax when targeting Node.js 10
const handlePromise = () => pFinally(Promise.all([
processDone,
getStream(spawned, 'stdout', {encoding, buffer, maxBuffer}),
getStream(spawned, 'stderr', {encoding, buffer, maxBuffer}),
getStream(spawned, 'all', {encoding, buffer, maxBuffer: maxBuffer * 2})
]).then(results => { // eslint-disable-line promise/prefer-await-to-then
const result = results[0];
result.stdout = results[1];
result.stderr = results[2];
result.all = results[3];

if (result.error || result.code !== 0 || result.signal !== null || isCanceled) {
const error = makeError(result, {
joinedCommand,
parsed,
timedOut,
isCanceled
});

// TODO: missing some timeout logic for killed
// https://github.com/nodejs/node/blob/master/lib/child_process.js#L203
// error.killed = spawned.killed || killed;
error.killed = error.killed || spawned.killed;
const handlePromise = () => {
let processComplete = Promise.all([
processDone,
getStream(spawned, 'stdout', {encoding, buffer, maxBuffer}),
getStream(spawned, 'stderr', {encoding, buffer, maxBuffer}),
getStream(spawned, 'all', {encoding, buffer, maxBuffer: maxBuffer * 2})
]);

if (timeoutId) {
processComplete = Promise.race([
processComplete,
resolvable
]);
}

if (!parsed.options.reject) {
return error;
return pFinally(processComplete.then(results => { // eslint-disable-line promise/prefer-await-to-then
GMartigny marked this conversation as resolved.
Show resolved Hide resolved
const result = results[0];
result.stdout = results[1];
result.stderr = results[2];
result.all = results[3];

if (result.error || result.code !== 0 || result.signal !== null || isCanceled) {
const error = makeError(result, {
joinedCommand,
parsed,
timedOut,
isCanceled
});

// TODO: missing some timeout logic for killed
// https://github.com/nodejs/node/blob/master/lib/child_process.js#L203
// error.killed = spawned.killed || killed;
error.killed = error.killed || spawned.killed;

if (!parsed.options.reject) {
return error;
}

throw error;
}

throw error;
}

return {
stdout: handleOutput(parsed.options, result.stdout),
stderr: handleOutput(parsed.options, result.stderr),
all: handleOutput(parsed.options, result.all),
code: 0,
exitCode: 0,
exitCodeName: 'SUCCESS',
failed: false,
killed: false,
command: joinedCommand,
timedOut: false,
isCanceled: false
};
}), destroy);
return {
stdout: handleOutput(parsed.options, result.stdout),
stderr: handleOutput(parsed.options, result.stderr),
all: handleOutput(parsed.options, result.all),
code: 0,
exitCode: 0,
exitCodeName: 'SUCCESS',
failed: false,
killed: false,
command: joinedCommand,
timedOut: false,
isCanceled: false
};
}), destroy);
};

crossSpawn._enoent.hookChildProcess(spawned, parsed.parsed);

Expand Down
19 changes: 16 additions & 3 deletions test.js
View file
Open in desktop
Expand Up @@ -385,15 +385,28 @@ test('error.code is 2', code, 2);
test('error.code is 3', code, 3);
test('error.code is 4', code, 4);

test('timeout will kill the process early', async t => {
const error = await t.throwsAsync(execa('delay', ['60000', '0'], {timeout: 1500, message: TIMEOUT_REGEXP}));
test.serial('timeout will kill the process early', async t => {
const time = Date.now();
const error = await t.throwsAsync(execa('delay', ['60000', '0'], {timeout: 500, message: TIMEOUT_REGEXP}));
const diff = Date.now() - time;

t.true(error.timedOut);
t.not(error.exitCode, 22);
t.true(diff < 4000);
});

test.serial('timeout will kill the process early (sleep)', async t => {
const time = Date.now();
const error = await t.throwsAsync(execa('sleeper', [], {timeout: 500, message: TIMEOUT_REGEXP}));
const diff = Date.now() - time;

t.true(error.timedOut);
t.not(error.stdout, 'ok');
t.true(diff < 4000);
});

test('timeout will not kill the process early', async t => {
const error = await t.throwsAsync(execa('delay', ['3000', '22'], {timeout: 30000}), {code: 22, message: getExitRegExp('22')});
const error = await t.throwsAsync(execa('delay', ['2000', '22'], {timeout: 30000}), {code: 22, message: getExitRegExp('22')});
t.false(error.timedOut);
});

Expand Down