FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: Warn against `uncaughtException` dependency. by lance · Pull Request #6378 · nodejs/node · GitHub

/ node Public

doc: Warn against uncaughtException dependency. - #6378

Closed
lance wants to merge 1 commit into
nodejs:masterfrom
lance:uncaught-exception-doc
ConversationCommits (1)ChecksFiles changed
Closed

doc: Warn against uncaughtException dependency.#6378
lance wants to merge 1 commit into
nodejs:masterfrom
lance:uncaught-exception-doc

Conversation

lance commented Apr 25, 2016

Copy link
Copy Markdown
Member
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

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
Comment thread doc/api/process.md Outdated

cjihrig Apr 25, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

lance Apr 25, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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."?

cjihrig Apr 25, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

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

addaleax commented Apr 28, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell reviewed Apr 29, 2016
View reviewed changes
Comment thread doc/api/process.md Outdated

jasnell Apr 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell commented Apr 29, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell commented Apr 29, 2016

Copy link
Copy Markdown
Member

@nodejs/documentation

stevemao commented May 4, 2016

Copy link
Copy Markdown
Contributor

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

sam-github reviewed May 4, 2016
View reviewed changes
Comment thread doc/api/process.md Outdated

sam-github May 4, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lance May 4, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented May 4, 2016

Copy link
Copy Markdown
Member Author

@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 commented May 5, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented May 27, 2016

Copy link
Copy Markdown
Member

Ping... any further thoughts on this?

lance commented May 27, 2016

Copy link
Copy Markdown
Member Author

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 commented Jul 14, 2016

Copy link
Copy Markdown
Member Author

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

JacksonTian commented Jul 14, 2016

Copy link
Copy Markdown
Contributor

Could you merge commits into one.

doc: Warn against uncaughtException dependency.
bc1f7d2 
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 commented Jul 14, 2016

Copy link
Copy Markdown
Member Author

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

cjihrig commented Jul 14, 2016

Copy link
Copy Markdown
Contributor

LGTM

lance commented Jul 15, 2016

Copy link
Copy Markdown
Member Author

Landed in: 2fe277a

lance closed this Jul 15, 2016
lance added a commit that referenced this pull request Jul 15, 2016
doc: Warn against uncaughtException dependency.
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 <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #6378
evanlucas pushed a commit that referenced this pull request Jul 19, 2016
doc: Warn against uncaughtException dependency.
c726ffb 
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 <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #6378
evanlucas mentioned this pull request Jul 19, 2016
Merged
evanlucas pushed a commit that referenced this pull request Jul 20, 2016
doc: Warn against uncaughtException dependency.
9c274e3 
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 <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #6378
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL