doc: exit event is not guaranteed to fire #2861

Trott · 2015-09-14T17:31:30Z

This change:

notes that the exit event is not guaranteed to fire
provides an example situation where the exit event may not fire
makes a minor copyediting change
enforces 80 character wrap in one place where it was not honored

reqshark · 2015-09-14T17:57:00Z

doc/api/process.markdown

@@ -59,6 +59,9 @@ finished running the process will exit. Therefore you **must** only perform
 checks on the module's state (like for unit tests). The callback takes one
 argument, the code the process is exiting with.

+This event is not guaranteed to fire in all situations. For example, if the
+process receives `SIGINT`, it may exit without firing this event.


@Trott, please review line 247 below. It's the last bullet point under the section on Signal Events

I might say something like:

The exit event will not fire if the process receives one of three program interrupts intended to override the runtime regardless of call stack depth, i.e. SIGINT, SIGTERM, or SIGKILL.

I thought the last bullet point might be specific to this: Note that Windows does not support sending Signals, but Node.js offers some emulation with process.kill(), and child_process.kill():

So I wasn't sure that it applied in all cases, or if it was specific to signals received via process.kill() and child_process.kill(). On reflection, that doesn't really make any sense. I wonder if that sentence is actually supposed to have a period at the end rather than a colon.

So yeah, I'll try to edit it a bit more.

It would seem that the list of interrupts that bypass the exit event is not limited to just those three. SIGHUP seems to do it too, for example.

good point. that's still just another user shutdown like ctrl + c stuff. here's a list:

http://www.gnu.org/software/libc/manual/html_node/Termination-Signals.html

well no nevermind, it's a much different signal from ctrl + c stuff.

SIGHUP is any hangup, so while it could happen in the terminal, i think it could also occur from any socket opened by http or net or a binding, etc

bnoordhuis · 2015-09-14T20:29:44Z

doc/api/process.markdown

 - Sending `SIGINT`, `SIGTERM`, and `SIGKILL` cause the unconditional exit of the
  target process.

+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 search for the existence of a process


s/search/check/?

EDIT: Or 'test', like below.

bnoordhuis · 2015-09-14T20:32:08Z

If you use 'refine' as the short description, you should have a long description that explains what. :-)

LGTM apart from that and a suggestion.

This change: * notes that the exit event is not guaranteed to fire * provides an example situation where the exit event may not fire * makes a minor copyediting change * enforces 80 character wrap in one place where it was not honored Fixes: nodejs#2853

ronkorving · 2015-09-15T01:30:58Z

doc/api/process.markdown

@@ -218,7 +221,7 @@ Note:
  the terminal mode before exiting with code `128 + signal number`. If one of
  these signals has a listener installed, its default behavior will be removed
  (Node.js will no longer exit).
- `SIGPIPE` is ignored by default, it can have a listener installed.
+- `SIGPIPE` is ignored by default. It can have a listener installed.


Unrelated to this PR, but is this a recent thing? I remember having to use https://www.npmjs.com/package/epipebomb in the past to prevent rare, but real issues from occurring.

git blame says that line was introduced in the docs in October 2013 with 155df9ca7. The behavior predates the documentation of it, but I don't know by how much.

Can you confirm if that means that epipebomb is therefore useless?

Using current node (4.0.0), I'm able to trigger EPIPE on OS X with:

node -e 'for (var i = 0; i < 100; i++) console.log(i)' | head

If I install epipebomb and require it first, it doesn't fix anything.

So, I guess it's useless now? But you can still get EPIPE errors? Not sure.

Doesn't that mean that the documentation saying "SIGPIPE is ignored by default" is wrong? I think it would be nice if that would just be solved in Node once and for all (not in docs).

So it's still needed :(
Is there any reason why that is not default behavior?

I don't think it's wrong. Writing to a pipe that can't read results in both SIGPIPE signal and EPIPE error. The signal is ignored while the error is not. At least, that's how I'm seeing it. Correction welcome. And I suspect the docs can be clarified on that. (Feel free to PR something better if you have a good idea.)

The signal is ignored while the error is not

@Trott wait a minute, what do you mean by ignoring the signal?

also what do you think about my idea from just a few minutes ago in the issue discussion? #2853 (comment)

I'm probably going to stop after this comment because there are people who are waaaaay more informed than I am about how this works in Node and why. If I keep going without doing some homework, I'll probably only create annoyance and/or confusion.

@reqshark I used the word "ignore" because that's what's in the docs. I meant nothing precise by it.

@ronkorving I agree that it's surprising that the epipebomb-enabled behavior isn't the default as that seems to be the default everywhere else (judging from the fact that I've never run up against it piping output through head from Perl, Python, Ruby, what have you).

yo @Trott just add this bookmark, https://www.gnu.org/software/libc/manual/html_mono/libc.html

SIGPIPE is a signal from the kernel. EPIPE is an error code. In the linked issue discussion I was talking more about termination signals (not EPIPE nor its conjoined twin, SIGPIPE)

Trott · 2015-09-16T20:55:39Z

Going to go and merge this on the grounds that there is a LGTM and it is an improvement over the current docs. If other conversation around this issue should continue, it can do so in other tickets (probably either #2853 or a new ticket opened specifically for the purpose).

This change: * notes that the exit event is not guaranteed to fire * provides an example situation where the exit event may not fire * makes a minor copyediting change * enforces 80 character wrap in one place where it was not honored Fixes: #2853 PR-URL: #2861 Reviewed-By: Ben Noordhuis <[email protected]>

Trott · 2015-09-16T20:58:53Z

Landed in 43cb1dd

sam-github · 2015-09-16T21:30:10Z

@Trott I don't think merging something in over objections with the claim that its an improvement is really the way to go. Particularly when both meaningful changes it introduces have been contested as inaccurate.

Even if this was a vote, you have one +, one -, and that's not an overwhelming endorsement. I wrote the docs you are changing, I have some familiarity with them! Of course, Ben probably wrote the code, but he knows that SIGINT does not unconditionally cause exit of a target process on Unix, I think he didn't notice that you'd decontexualized that statement. @bnoordhuis, comment?

Saying that an event "may be fired" is not particularly useful. Under what conditions will it be emitted or not emitted?

I would ask that you revert this, reopen it, and continue the discussion.

/cc @rvagg We were discussing premature closing of PRs a few hours ago... I am thinking there needs to be more clarity on how fast things are closed.

Trott · 2015-09-16T21:36:59Z

@sam-github I apologize about that. I didn't see your comments. (They got lost in the inline comments. That's not an excuse, just an explanation. I should have been more meticulous.) Had I seen your comments, I would not have merged.

I've opened #2918 to try to remediate, but if you'd prefer a full-on revert of these changes, I'm happy to do that (or let someone else do it).

sam-github · 2015-09-16T21:49:24Z

OK, I see, things get lost in github sometimes, that makes more sense.

I think just making the statement about when the exit event happens more definite and making sure the always terminate statement is recontextualized is enough.

How you do that is up to you, don't revert unless that makes it easy for you.

reqshark · 2015-09-16T22:48:36Z

One exit handler would be more consistent with the idea of an exit event handler.

@sam-github, docs wont solve that.

sam-github · 2015-09-16T22:56:22Z

@reqshark I'm missing some context, what do you mean by one exit handler? There is only one exit event. And what "that" are you refering to?

reqshark · 2015-09-16T23:06:27Z

no there are plenty of other exit-like events.

these other process exits or termination events bypass the exit event handler; they go by other names.

process.on('exit', function() {
  console.log('this never happens');
})

process.on('SIGHUP', function () {
  console.log('however this prevents exit')
});

I recently moved from New York City to the Peninsula, about 20 minutes south of San Francisco. While the area is nice, I often see streets that go by more than one name. Arbitrarily the city planners around here will give more than one name to a street and change the name at a crossing or at a certain stoplight. That's basically what process.on('exit' is doing in node.js

reqshark · 2015-09-16T23:17:31Z

also @sam-github, that was unclear, sorry i never meant "exit handler", i just mean "exit event"

This change: * notes that the exit event is not guaranteed to fire * provides an example situation where the exit event may not fire * makes a minor copyediting change * enforces 80 character wrap in one place where it was not honored Fixes: nodejs#2853 PR-URL: nodejs#2861 Reviewed-By: Ben Noordhuis <[email protected]>

Trott added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Sep 14, 2015

reqshark reviewed Sep 14, 2015
View reviewed changes

Trott force-pushed the doc-process-updates branch from 789276d to f4e001c Compare September 14, 2015 19:54

bnoordhuis reviewed Sep 14, 2015
View reviewed changes

Trott force-pushed the doc-process-updates branch from b65d753 to a1165b4 Compare September 14, 2015 20:35

Trott force-pushed the doc-process-updates branch from a1165b4 to 2334be6 Compare September 14, 2015 20:37

ronkorving reviewed Sep 15, 2015
View reviewed changes

Trott closed this Sep 16, 2015

Trott mentioned this pull request Sep 16, 2015

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

Closed

rvagg mentioned this pull request Sep 22, 2015

Release proposal: v4.1.1 #2995

Merged

Trott deleted the doc-process-updates branch January 9, 2022 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: exit event is not guaranteed to fire #2861

doc: exit event is not guaranteed to fire #2861

Trott commented Sep 14, 2015

reqshark Sep 14, 2015

Trott Sep 14, 2015

Trott Sep 14, 2015

reqshark Sep 14, 2015

reqshark Sep 14, 2015

bnoordhuis Sep 14, 2015

bnoordhuis commented Sep 14, 2015

ronkorving Sep 15, 2015

Trott Sep 15, 2015

ronkorving Sep 15, 2015

Trott Sep 15, 2015

ronkorving Sep 15, 2015

ronkorving Sep 15, 2015

Trott Sep 15, 2015

reqshark Sep 15, 2015

Trott Sep 15, 2015

reqshark Sep 15, 2015

Trott commented Sep 16, 2015

Trott commented Sep 16, 2015

sam-github commented Sep 16, 2015

Trott commented Sep 16, 2015

sam-github commented Sep 16, 2015

reqshark commented Sep 16, 2015

sam-github commented Sep 16, 2015

reqshark commented Sep 16, 2015

reqshark commented Sep 16, 2015

doc: exit event is not guaranteed to fire #2861

doc: exit event is not guaranteed to fire #2861

Conversation

Trott commented Sep 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Sep 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Sep 16, 2015

Trott commented Sep 16, 2015

sam-github commented Sep 16, 2015

Trott commented Sep 16, 2015

sam-github commented Sep 16, 2015

reqshark commented Sep 16, 2015

sam-github commented Sep 16, 2015

reqshark commented Sep 16, 2015

reqshark commented Sep 16, 2015