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

doc: refine process.kill() and exit explanations #2918

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 16, 2015

R: @sam-github, @nodejs/documentation
CC: @ronkorving
Ref: #2861
Fix: #2924

@Fishrock123 Fishrock123 added the doc Issues and PRs related to the documentations. label Sep 16, 2015
@sam-github
Copy link
Contributor

@Trott this addresses all my concerns, thank you for that, LGTM from me

@sam-github
Copy link
Contributor

I'm not sure how fast this can be merged, the other PR was open a couple days, and only @bnoordhuis and I commented, so maybe this could merge quickly, or maybe you should wait until tomorrow in case someone does have opinions.

@Trott
Copy link
Member Author

Trott commented Sep 16, 2015

@sam-github: I was just typing this:

I'll land this Wed 17-Sep around 3PM PDT/10PM UMT unless there are objections.

If you feel really good about it and want to fast-track land it sooner, I don't have a problem with that. If you leave it to me, I'm inclined to give it 24 hours.


Note that Windows does not support sending Signals, but Node.js offers some
emulation with `process.kill()`, and `child_process.kill()`. Sending signal `0`
can be used to test for the existence of a process
can be used to test for the existence of a process. Sending `SIGINT`,
`SIGTERM`, and `SIGKILL` cause the unconditional exit of the target process.
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless an event handler is listening, in the case of SIGINT and SIGTERM, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes on POSIX systems, but no on Windows. That paragraph applies to Windows only.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, thanks. It does sound to me then that inevitably, Node should strive to unify the behavior. If we can't get Windows to behave, perhaps we need to downgrade the POSIX behavior? (that would suck, but if that's the only way ...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey, @sam-github, should I change exit in line 248 to termination?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, Windows doesn't support signals, so it doesn't support the distinction, but its still more consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, done.

@sam-github
Copy link
Contributor

LGTM

@Trott
Copy link
Member Author

Trott commented Sep 17, 2015

Removed the for example part. Those details can go elsewhere, preferably somewhere that they can be explained comprehensively, such as a guide at https://github.com/nodejs/docs/tree/master/src

Trott added a commit to Trott/io.js that referenced this pull request Sep 17, 2015
Add corrections about when exit event fires and how .kill() works on
Windows.

PR-URL: nodejs#2918
Reviewed-By: Sam Roberts <[email protected]>
@Trott
Copy link
Member Author

Trott commented Sep 17, 2015

Landed in 4dcf24c

@Trott Trott closed this Sep 17, 2015
Trott added a commit that referenced this pull request Sep 20, 2015
Add corrections about when exit event fires and how .kill() works on
Windows.

PR-URL: #2918
Reviewed-By: Sam Roberts <[email protected]>
@rvagg rvagg mentioned this pull request Sep 22, 2015
@ronkorving
Copy link
Contributor

Thanks @Trott. I was afk for a few weeks, so was unable to respond for a while, but appreciate the changes 👍

@Trott Trott deleted the further-doc-process branch January 9, 2022 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants