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

Don't recommend domains for error handling #2056

Closed
wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

This PR closes #2055 by removing the suggestion to use domains for exception handling. Additionally clarify unhandledException a bit.

PR-URL: #2056

@Fishrock123 Fishrock123 added the doc Issues and PRs related to the documentations. label Jun 25, 2015
@trevnorris
Copy link
Contributor

Not saying it's not, but it should be clear that unhandledException should not be used for error recovery.

@benjamingr
Copy link
Member Author

Do not use it as the io.js equivalent of On Error Resume Next. An
unhandled exception means your application - and by extension io.js itself -
is in an undefined state. Blindly resuming means anything could happen.

Think of resuming as pulling the power cord when you are upgrading your system.
Nine out of ten times nothing happens - but the 10th time, your system is bust.

Improvement suggestions? Seems az clear as it gets to me :)

@targos
Copy link
Member

targos commented Jun 25, 2015

The "On Error Resume Next" is a strange to me. I understand what it means but I had to google to know it is a VB concept.

@benjamingr
Copy link
Member Author

@targos , I actually didn't change that part but I quite like it, I understood it but I never professionally did VB. I don't mind changing it while I'm at it if you have a suggestion :)

@albertorestifo
Copy link

What about:

Do not use it as an error handling middleware.

@benjamingr
Copy link
Member Author

I'd rather not impose words like middleware that not everyone knows on the documentation, if someone is just now exploring what unhandledRejection is I'm not sure they'd understand that.

@albertorestifo
Copy link

Agree. What about:

Do not use it to attempt error recovery. An unhandled exception means your application - and by extension io.js itself - is in an undefined state. Blindly resuming means anything could happen.

@benjamingr
Copy link
Member Author

@albertorestifo

An unhandled exception means your application - and by extension io.js itself - is in an undefined state. Blindly resuming means anything could happen.

Think of resuming as pulling the power cord when you are upgrading your system. Nine out of ten times nothing happens - but the 10th time, your system is bust.

You have been warned.

This seems pretty strong already to me. Wouldn't you agree?

@albertorestifo
Copy link

@benjamingr 👍

@benjamingr
Copy link
Member Author

Can anyone else LGTM and/or merge it?

@trevnorris
Copy link
Contributor

LGTM

@reqshark
Copy link

+1

It is mainly useful to perform synchronous cleanup when the server is at a
problematic state in order to gracefully shut down.

If you do use it, restart your application after every unhandled exception!

Choose a reason for hiding this comment

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

this sounds right but I might say:

Handling this type of exception recognizes an unrecoverable problem with the application, at which point it is time to both: restart the application and investigate further.

Copy link
Member Author

Choose a reason for hiding this comment

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

"Recognizes an unrecoverable problem" sounds a bit unclear to me, do you mind rephrasing?

Choose a reason for hiding this comment

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

hmm, if that's unclear, I guess the main point there is that such an exception shouldn't be held out as a crutch to blindly restart applications, carrying on as if nothing happened, but rather an occasion to re-evaluate some area that is unfinished or very likely broken

This PR closes nodejs#2055 by removing the suggestion to use domains for exception handling. Additionally clarify `unhandledException` a bit.

PR-URL: nodejs#2056
Reviewed-By: Trev Norris <trev.norris@gmail.com>
Reviewed-By: Chris Dickinson <chris@neversaw.us>
@benjamingr
Copy link
Member Author

Sorry for the time it took, squished, added Reviewed-By to commit message.

trevnorris pushed a commit that referenced this pull request Jul 3, 2015
Remove the suggestion to use domains for exception handling. Add clarity
to "unhandledException".

Fixes: #2055
PR-URL: #2056
Reviewed-By: Trev Norris <trev.norris@gmail.com>
Reviewed-By: Chris Dickinson <chris@neversaw.us>
@trevnorris
Copy link
Contributor

Landed with git commit message fix, and removal of trailing whitespace on 0f09b8d.

@trevnorris trevnorris closed this Jul 3, 2015
mscdex pushed a commit to mscdex/io.js that referenced this pull request Jul 9, 2015
Remove the suggestion to use domains for exception handling. Add clarity
to "unhandledException".

Fixes: nodejs#2055
PR-URL: nodejs#2056
Reviewed-By: Trev Norris <trev.norris@gmail.com>
Reviewed-By: Chris Dickinson <chris@neversaw.us>
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.

Documentation: process event uncaughtException pointing to deprecated module
7 participants