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

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

Merged
merged 4 commits into from Apr 12, 2019

Conversation

GMartigny
Copy link
Contributor

Fix #157
Add regression test

Copy link
Collaborator

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

This looks good, except the tests are failing on CI.

index.js Show resolved Hide resolved
@ehmicky
Copy link
Collaborator

ehmicky commented Apr 3, 2019

Thanks a lot for the PR Guillaume! I've just reviewed it. Looking forward to merge this fix.

@GMartigny
Copy link
Contributor Author

As for the tests, they're working on local. I could remove the "leaving early" test. I thought that checking 2000ms over 500ms expected was safe enough. I'll try to extends the margin first, but I might have to remove this test (which remove the regression test).

@ehmicky
Copy link
Collaborator

ehmicky commented Apr 3, 2019

You can try to use test.serial() as well to ensure no other tests are running concurrently. This will ensure more predictable timing.

@ehmicky
Copy link
Collaborator

ehmicky commented Apr 3, 2019

This looks good to me now. Just waiting for @sindresorhus review before merging it :)

Thanks! 👏

index.js Outdated Show resolved Hide resolved
@@ -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.

@sindresorhus sindresorhus merged commit 136b340 into sindresorhus:master Apr 12, 2019
@sindresorhus
Copy link
Owner

This is looking good. Thanks for contribution, @GMartigny 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

timeout option is not considered if streams are attached
3 participants