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

Uncaught exceptions in Promises inherently dangerous? #5254

Closed
taylorhakes opened this issue Feb 16, 2016 · 5 comments
Closed

Uncaught exceptions in Promises inherently dangerous? #5254

taylorhakes opened this issue Feb 16, 2016 · 5 comments
Labels
process Issues and PRs related to the process subsystem. question Issues that look for answers.

Comments

@taylorhakes
Copy link

I have been reading a lot about Node uncaught exception handling. Information in the docs here talks about logging uncaught errors and immediately exiting the process. I also found similar information in the domain docs. Essentially, your Node process is in an unknown state and the only safe thing to do is restart.

I then discovered a related issue discussion about the dangers of process.on('uncaughtException'
nodejs/node-v0.x-archive#2582

Since Promises catch errors (known and unknown) and there are no guards on Promise.prototype.catch to filter error types, any Promise code, even with a .catch, could be leaking references similar to the issues with domain and uncaughtException

There has been a recent discussion here about what to do on process.on('unhandledRejection'
#830

It appears this issue is bigger than just unhandled rejections. All Promise code in Node appears to be dangerous? Or am I missing something?

@mscdex mscdex added question Issues that look for answers. process Issues and PRs related to the process subsystem. labels Feb 16, 2016
@misterdjules
Copy link

@taylorhakes This doesn't seem to be describing a bug/issue with node itself, so I'm closing this issue.

However, I would invite you to participate in/start discussions at https://github.com/nodejs/promises, where such questions are currently actively discussed.

Thank you!

@petkaantonov
Copy link
Contributor

Promises only catch errors that are catchable with a try-catch block to translate them into rejections, which will end up in process.on('unhandledRejection' if you don't handle the rejection. They cannot catch exceptions that would end up in domains or process 'uncaughtException', unless you use those events yourself to reject promises yourself.

@petkaantonov
Copy link
Contributor

Although ugly, repetitive and error-prone, you can have the equivalent of catch guards by rethrowing the error

.catch(function(e) {
   if (e.code === "ENOENT") {

   } else {
        throw e;
   }
});

@taylorhakes
Copy link
Author

@petkaantonov Thanks for response. Without Promises (using callbacks), those errors would go to uncaughtException.

Although ugly, repetitive and error-prone, you can have the equivalent of catch guards by rethrowing the error

That is one of the biggest pitfalls with Promises I have seen. Most developers do not do that check in the .catch. Also it is error prone as you said. If you have multiple identical errors up the chain you can never know what you are catching. I have a very hard time understanding the need to catch programming errors. It seems to cause more issues than it solves for me. That behavior obviously can't change at this point.

On a side note, I am not a fan of creating more exceptions. I try to limit them so I can find real exceptions. I would change your above code to this.

.catch(function(e) {
   if (e.code === "ENOENT") {

   } else {
       return Promise.reject(e);
   }
});

It is slightly slower (another Promise), but no exception. Or you could use the catch guards in Bluebird :)

@petkaantonov
Copy link
Contributor

I think returning Promise.reject is actually much faster than throwing (in bluebird at least), it's just that catching error is not the common case so I am not too careful about performance in those cases and throw e is shorter to write.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

4 participants