Skip to content
This repository has been archived by the owner on Mar 3, 2020. It is now read-only.

Unhandled promises exception handler #1

Closed
thetutlage opened this issue Oct 29, 2017 · 6 comments
Closed

Unhandled promises exception handler #1

thetutlage opened this issue Oct 29, 2017 · 6 comments
Assignees

Comments

@thetutlage
Copy link
Member

From @benallfree on October 28, 2017 23:8

Right now we don't output stack traces for unhandled promises.

nodejs/node#9523 (comment)

process.on('unhandledRejection', r => console.log(r))

Fixes it.

Is this something that is worth adding to the framework when in dev mode?

Copied from original issue: adonisjs/core#688

@thetutlage
Copy link
Member Author

Yes, we can add this. But with a bold warning saying that all async/await should be wrapped inside try/catch and Promises should have .catch on them

@benallfree
Copy link
Contributor

Laravel solves this with a global exception handler as last resort. Of course users can wrap in try/catch if they wish, but I hope this handler would keep the process from exiting in production and log the error (or display it?), and in dev mode just output a stack trace like the sample above. What do you think?

@thetutlage
Copy link
Member Author

Yeah so in Node land, we prefer the process to crash for uncaught exceptions. So here are 2 things.

  1. Uncaught exceptions
  2. Unhandled rejections

The first one is something blowed somewhere and no one handled it. So we want the process to crash, so that there are no weird memory leaks.
https://nodejs.org/docs/latest/api/process.html#process_event_uncaughtexception


The unhandled rejections is more of, dude you are writing async code and expecting that it will never reject, but that is wrong.

We should add a listener for it, but only for educating the user that there is something wrong and but not for rescuing them from a bad situation.

@benallfree
Copy link
Contributor

benallfree commented Oct 29, 2017

That would work for me. Like we run it in dev mode and each time it fires it shows a big warning educating developers about what to do.

To be fair, at dev time, most of my exceptions have to do with buggy code, not actually legitimate runtime rejections :)

WARNING: Adonis has detected an unhandled rejection that will
cause node to exit in a live environment. Examine the cause of
this rejection. If it might occur in a live environment,
use try/catch to gracefully handle it. If it is a programming error,
correct it now. 

@thetutlage
Copy link
Member Author

Yes, will do this

@benallfree
Copy link
Contributor

Fine work!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants