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

Rethrow only on node v0.8. #19

Merged
merged 2 commits into from
Nov 29, 2013
Merged

Conversation

mcollina
Copy link
Contributor

This pull-request uses process.versions.node to detect if we are on node v0.8 and only in that case we rethrow after the 'uncaughtException' handler.
This solves the problem in #14 in a much simpler way.

@mcollina
Copy link
Contributor Author

done!

@mcollina
Copy link
Contributor Author

@raszi could you please review this?

@raszi
Copy link
Owner

raszi commented Nov 29, 2013

Actually I wanted to change the whole behavior on this. I started to reimplement the graceful cleanup with domains but it did not help.

raszi added a commit that referenced this pull request Nov 29, 2013
@raszi raszi merged commit 1855479 into raszi:master Nov 29, 2013
@raszi
Copy link
Owner

raszi commented Nov 29, 2013

So I'll accept this I finish the work with that.

@raszi
Copy link
Owner

raszi commented Nov 29, 2013

Released as v0.0.22, thanks!

@mcollina mcollina deleted the throw-only-on-v0.8 branch November 29, 2013 14:54
@mcollina
Copy link
Contributor Author

Thank you! :)

@wibblymat
Copy link
Contributor

Correct me if I'm wrong... but doesn't this mean that if I use tmp in my project then any uncaught exception will just disappear without a trace in node 0.10? If so then this is much worse behaviour than before. Debugging will be a nightmare.

@mcollina
Copy link
Contributor Author

mcollina commented Dec 2, 2013

Ok, I saw the problem. It didn't happen in my code because I use mocha for testing (which have its own uncaughtException handler) and domains (which use uncaughtException). So, the error now it's not reported if there is no uncaughtException handler.
This assumption is based on this comment: #14 (comment).

@mcollina
Copy link
Contributor Author

mcollina commented Dec 2, 2013

The fix is changing that line into:

  if (process.versions.node.indexOf('0.8') == 0 || process.listeners('uncaughtException').length === 1)

@mcollina
Copy link
Contributor Author

mcollina commented Dec 2, 2013

However, that fix do not change the issue of bad error reporting.

I've noted in the code that there are both a 'uncaughtException' and 'exit' event handlers that do almost the same thing. Why do we need both? How about removing the 'uncaughtException' once and for all? All remove operations are sync.

@wibblymat
Copy link
Contributor

See #21. In node 0.8 the exit event won't fire if you are exiting with an exception so you still need uncaughtException for that case.

raszi added a commit that referenced this pull request May 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants