Skip to content

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

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 <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
@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 <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
@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 <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
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