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

Improve error.killed #227

Merged
merged 2 commits into from
May 10, 2019
Merged

Improve error.killed #227

merged 2 commits into from
May 10, 2019

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented May 10, 2019

This changes the conditions behind error.killed.

a) childProcess.kill() was successfully called

true in core Node.js, true in execa. Unchanged.

b) process.kill(childProcess.pid) was called

false in core Node.js, false in execa. Unchanged.

Ideally this should be true, but I am not sure that's possible. The issue is that Windows uses exitCode: 1 and no signal, while Unix uses exitCode: null and signal: 'SIGTERM'. The difference is due to Node.js emulating termination signals on Windows. On Windows (with just exitCode: 1) it seems impossible to know whether the child process was killed externally.

One consequence of this is: while process.kill(childProcess.pid) will set error.killed to false on both Windows and Unix, the error message will be different: was killed with SIGTERM on Unix, failed with exit code 1 on Windows.

c) timeout was reached

true in core Node.js, true in execa. Changed to false with this PR. We have error.timedOut for this. This makes those boolean error properties orthogonal to each other.

d) maxBuffer was reached

true in core Node.js, false in execa. Unchanged. We should add a property similar to error.timedOut but for maxBuffer instead. I can do it in a separate PR.

Note: this not does change childProcess.killed, which is a separate property and is true when childProcess() was successfully called (in both core Node.js and execa).

@ehmicky ehmicky requested a review from sindresorhus May 10, 2019 10:32
@ehmicky ehmicky force-pushed the feature/error-killed branch 2 times, most recently from 9ee017b to f69b579 Compare May 10, 2019 11:12
@sindresorhus
Copy link
Owner

Can you fix the merge conflict?

@sindresorhus
Copy link
Owner

On Windows (with just exitCode: 1) it seems impossible to know whether the child process was killed externally.

Should we open an Node.js issue about that?

@ehmicky
Copy link
Collaborator Author

ehmicky commented May 10, 2019

Merge conflict fixed.

I have opened an issue on Node. However I think this PR could still be merged regardless: if that issue resolves, we can create a new PR so that error.killed is true on external signal termination.

@sindresorhus sindresorhus merged commit b5dfb0b into master May 10, 2019
@sindresorhus sindresorhus deleted the feature/error-killed branch May 10, 2019 16:42
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.

2 participants