doc: Warn against `uncaughtException` dependency. #6378

lance · 2016-04-25T19:52:42Z

Checklist

documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

State in the documentation that uncaughtException is not a reliable
way to restart a crashed application, and clarify that an application
may crash in ways that do not trigger this event.

Fixes #6223

cjihrig · 2016-04-25T20:31:23Z

doc/api/process.md

@@ -129,7 +129,8 @@ unforeseen and unpredictable issues.

 Exceptions thrown from within the event handler will not be caught. Instead the
 process will exit with a non zero exit code and the stack trace will be printed.
-This is to avoid infinite recursion.
+This is to avoid infinite recursion. There may be other situations where the


Some clarification on the "other situations" would be nice. After reading this, people might not be clear whether or not their uncaughtException handler can be expected to run or not.

Well, I think the point is that it is not guaranteed to run for all application 'crashes', where 'crash' is pretty loosely defined. I can send $ kill -9 NODE_PID from the command line to my Node.js process, and the uncaughtException event is never emitted. Granted, there is no exception in this case, but I think that's what the docs are intended to mean. A crash does not necessarily mean uncaughtException is emitted.

How about, "There may be other situations where the application could crash and not trigger this event. For example, receiving a SIGKILL will cause the application to exit abruptly, but an uncaughtException event will not be emitted."?

Maybe you should just amend the first sentence in the section that describes what 'uncaughtException' event is. Currently it says:

The 'uncaughtException' event is emitted when an exception bubbles all the way back to the event loop.

You could change it to say something like:

The 'uncaughtException' event is emitted when a thrown JavaScript exception bubbles all the way back to the event loop.

addaleax · 2016-04-28T16:26:28Z

LGTM

jasnell · 2016-04-29T05:31:17Z

doc/api/process.md

@@ -138,7 +139,9 @@ times nothing happens - but the 10th time, the system becomes corrupted.
 The correct use of `'uncaughtException'` is to perform synchronous cleanup
 of allocated resources (e.g. file descriptors, handles, etc) before shutting
 down the process. It is not safe to resume normal operation after


I would emphasize this line: **It is not safe to resume normal operation after 'uncaughtException'.**

jasnell · 2016-04-29T19:39:44Z

LGTM

jasnell · 2016-04-29T19:39:50Z

@nodejs/documentation

stevemao · 2016-05-04T06:02:25Z

Linking nodejs/docs#82
I believe people have different opinions on this.

sam-github · 2016-05-04T14:09:40Z

doc/api/process.md

-down the process. It is not safe to resume normal operation after
-`'uncaughtException'`.
+down the process. **It is not safe to resume normal operation after
+`'uncaughtException'`.**


I think "normal" needs to be defined, otherwise people are likely to interpret as "write the error into a remote mongodb and exit", or something of the like (see #6561). Its not safe to do anything async in the uncaughtException handler ... you can make some sync calls, and then throw the exception or exit.

With console.log being async... I'm not even sure how reasonable calling it is.

/cc @bnoordhuis

I agree, "normal" is pretty poorly defined here. But I think that's because "normal" can mean different things in different situations for different applications. I agree, however, that being explicit about async operations is probably good. This, and any other "you must not do X" rules would be great. This would allow developers to make their own decisions about fail-fast behavior or not - knowing the tradeoffs.

lance · 2016-05-04T16:34:14Z

@stevemao that was a fascinating read - thanks for the link. I'm not sure how that discussion should affect this doc change, other than perhaps a little more clarity about things like the async operations being verboten. Maybe that's the way I see it because I'm in the fail-fast camp, and see uncaughtException as being only minimally useful before allowing the app to crash.

stevemao · 2016-05-05T05:54:13Z

I believe most core devs like this approach. So you might want to add some notes about the browser behaviour. A browser always has a "hidden" uncaughtException attached, meaning an uncaught exception will never cause the app to crash, but only exists the current event loop.
I think this should clear things a little bit. It caused me a lot trouble before.
Just my 2c :)

jasnell · 2016-05-27T16:33:27Z

Ping... any further thoughts on this?

lance · 2016-05-27T20:10:12Z

So we now have

"The correct use of 'uncaughtException' is to perform synchronous cleanup of allocated resources (e.g. file descriptors, handles, etc) before shutting down the process. It is not safe to resume normal operation after 'uncaughtException'.".

Honestly, I think it's pretty clear, but I will add a final sentence to the docs after, if folks really think it's needed.

Maybe something like this?

"The correct use of 'uncaughtException' is to perform synchronous cleanup of allocated resources (e.g. file descriptors, handles, etc) before shutting down the process. It is not safe to resume normal operation after 'uncaughtException'. After uncaughtException is emitted, typical application behavior is to perform any necessary synchronous calls to clean up, and then throw an exception or exit."

lance · 2016-07-14T00:21:14Z

There are 2 LGTMs here, but I'm not sure if we're good to go. @jasnell @stevemao ?

JacksonTian · 2016-07-14T03:03:58Z

Could you merge commits into one.

State in the documentation that `uncaughtException` is not a reliable way to restart a crashed application, and clarify that an application may crash in ways that do not trigger this event. Use a documented synchronous function in example code. Fixes #6223

lance · 2016-07-14T14:26:57Z

@JacksonTian done. My editor did a little whitespace cleanup. I hope that's not an issue.

cjihrig · 2016-07-14T14:32:40Z

LGTM

lance · 2016-07-15T15:12:56Z

Landed in: 2fe277a

State in the documentation that `uncaughtException` is not a reliable way to restart a crashed application, and clarify that an application may crash in ways that do not trigger this event. Use a documented synchronous function in example code. Fixes: #6223 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> PR-URL: #6378

addaleax added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Apr 25, 2016

cjihrig reviewed Apr 25, 2016
View reviewed changes

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

jasnell reviewed Apr 29, 2016
View reviewed changes

sam-github reviewed May 4, 2016
View reviewed changes

lance closed this Jul 15, 2016

evanlucas mentioned this pull request Jul 19, 2016

v6.3.1 Release Proposal #7782

Merged

MylesBorins added the dont-land-on-v4.x label Sep 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Warn against `uncaughtException` dependency. #6378

doc: Warn against `uncaughtException` dependency. #6378

lance commented Apr 25, 2016

cjihrig Apr 25, 2016

lance Apr 25, 2016

cjihrig Apr 25, 2016

addaleax commented Apr 28, 2016

jasnell Apr 29, 2016

jasnell commented Apr 29, 2016

jasnell commented Apr 29, 2016

stevemao commented May 4, 2016

sam-github May 4, 2016

lance May 4, 2016

lance commented May 4, 2016

stevemao commented May 5, 2016 •

edited

Loading

jasnell commented May 27, 2016

lance commented May 27, 2016

lance commented Jul 14, 2016

JacksonTian commented Jul 14, 2016

lance commented Jul 14, 2016

cjihrig commented Jul 14, 2016

lance commented Jul 15, 2016

doc: Warn against uncaughtException dependency. #6378

doc: Warn against uncaughtException dependency. #6378

Conversation

lance commented Apr 25, 2016

Checklist

Affected core subsystem(s)

Description of change

cjihrig Apr 25, 2016

Choose a reason for hiding this comment

lance Apr 25, 2016

Choose a reason for hiding this comment

cjihrig Apr 25, 2016

Choose a reason for hiding this comment

addaleax commented Apr 28, 2016

jasnell Apr 29, 2016

Choose a reason for hiding this comment

jasnell commented Apr 29, 2016

jasnell commented Apr 29, 2016

stevemao commented May 4, 2016

sam-github May 4, 2016

Choose a reason for hiding this comment

lance May 4, 2016

Choose a reason for hiding this comment

lance commented May 4, 2016

stevemao commented May 5, 2016 • edited Loading

jasnell commented May 27, 2016

lance commented May 27, 2016

lance commented Jul 14, 2016

JacksonTian commented Jul 14, 2016

lance commented Jul 14, 2016

cjihrig commented Jul 14, 2016

lance commented Jul 15, 2016

doc: Warn against `uncaughtException` dependency. #6378

doc: Warn against `uncaughtException` dependency. #6378

stevemao commented May 5, 2016 •

edited

Loading