Node: Unhandled promise rejections are confusing to new users

[benjamingr]

I try to monitor and answer Node.js promise questions in StackOverflow. Several times now this sort of question arises where the OP gets an unhandled rejection error:

(node:52553) UnhandledPromiseRejectionWarning: Unhandled promise |  
rejection (rejection id: 1): ReferenceError: user is not defined |  
(node:52553) [DEP0018] DeprecationWarning: Unhandled promise > rejections are deprecated. In the future, promise rejections that are > not handled will terminate the Node.js process with a non-zero exit > code.

Which I then explain is just a regular error _wrapped_ in an UnhandledPromiseRejectionWarning.

I think changing the error format could really help the user understand the problem. Currently the average async/await user has to:

• Understand unhandled rejections and promise semantics to a reasonable degree.
• Understand that it's probably just an unhandled exception in promise land.
• Understand they need to run with --trace-warnings in order to get a reasonable stack trace for said error.

I think the current error format is pretty unwelcoming to new users and while it is a huge improvement over swallowing the error by default or not having a hook into it - we should improve it.

This issue is about the phrasing of the current warning and does not interfere with ongoing efforts to throw on unhandled rejections when they're detected in GC.

Does anyone have any better naming suggestions?

(Note, I'd prefer (if possible) that we keep using the existing warnings infrastructure for the warning)

[tniessen]

We could add a URL to a web page explaining the problem in more detail, some other frameworks do something similar when an internal error occurs.

[joyeecheung]

Maybe we should code the warnings and document them all in a central place, like what we do for errors?

[refack]

List of all [Ee]mitWarning I've found https://gist.github.com/refack/4fbb1e7411d10c82f6a18bd9556f70ab

[refack]

Does anyone have any better naming suggestions?

Looking at the current mechanism, seems like it should be easy to restructure the warning's message:

1. Always add stack trace for UnhandledPromiseRejectionWarning
2. Merge the first time deprecation warning, or separate the two better
   https://github.com/nodejs/node/blob/3a4f0e9b76d292f002edd8bd6e23815dc87d07ba/lib/internal/process/promises.js#L62-L84

[benjamingr]

https://github.com/nodejs/node/issues/16768 Another one

[benjamingr]

@refack thanks:

Expanding on your suggestion, maybe something like:

1. Always add stack trace for UnhandledPromiseRejectionWarning
2. Show the error first.
3. Explain it's in a promise chain.

Here is a bike chain:

(node:52553) Unhandled Async Error: ReferenceError: user is not defined
                               at Foo (foo.js:1:15)
                               at Bar (bar.js:10:15)
(node:52553) This error originated in an `async` function throwing or a rejected promise which was not handled in the code.
(node:52553) [DEP0018] DeprecationWarning: Unhandled promise rejections are dangerous. In the future, promise rejections that are not handled might terminate the Node.js process with a non-zero exit code.

WDYT @refack ?

Also looking for feedback from @jasnell who worked on the original warning API, @BridgeAR and @Fishrock123 for working on the "exit on GC" stuff and @addaleax for promise work and knowing these APIs very well.

[Fishrock123]

maybe loop in @mcollina too

fwiw my preference is: "exit process in nextTick if no event handler is registered"

[benjamingr]

fwiw my preference is: "exit process in nextTick if no event handler is registered"

The problem is that that are some people who are very strong opponents to that suggestion but won't voice their concern here.

Personally I'm really in favor of "warn after nextTick, exit on GC, exit after nextTick warning if wasn't handled for N seconds where N is large".

That said, this issue is more about the current warning message being confusing.

[mcollina]

My overall point on why any Node.js server should exits on unhandledRejection is listed in https://github.com/mcollina/make-promises-safe. The same very likely holds true for CLI tools. Both a) not printing the error stack trace and b) not exiting are definitely hindering the ability to use async/await and promises reliably in Node applications.

_when_ we exit is less of a constraint: I prefer exiting on nextTick, but exiting on gc is also ok. Consensus has eluded us on this topic for a long time, can we target this for 10.0.0?

[benjamingr]

when we exit is less of a constraint: I prefer exiting on nextTick, but exiting on gc is also ok. Consensus has eluded us on this topic for a long time, can we target this for 10.0.0?

I don't think exiting on nextTick is viable (although it's what I personally do often), consider:

async function foo(user) {
  var expensive = fetchUserInfoFromDb(user);
  var group = await fetchUserSmallInfoFromCache(user);
  // do some work on `group`
  // now we need the rest
  let userInfo = await expensive;
  await somethingOnBoth(userInfo, group);
}

I have to be very careful to _not_ write code like this and I suspect it's a footgun users might run into. Ideally I'd like promises inside an async function to _not_ throw unhandled rejection until the function is finished - GC can do that but has false negatives.

[mcollina]

@benjamingr I found that code is lacking intent of not caring about errors for a short span:

async function foo(user) {
  var expensive = fetchUserInfoFromDb(user);
  // we don't care about the error until we await
  expensive.catch(noop);
  var group = await fetchUserSmallInfoFromCache(user);
  // do some work on `group`
  // now we need the rest
  let userInfo = await expensive;
  await somethingOnBoth(userInfo, group);
}

function noop() {}

It's a tradeoff. On a side note, our current approach (warn) will actually result in a false positive warning in the case above. Not nice.

An incremental step for Node 10 is to start printing the full error message for every 'unhandledRejection' , but not crashing.

[refack]

As for the warning - I like @benjamingr's suggested format as above.
IMHO this _could_ be a semver minor change (for node8 & node9), and followed by upgrading the deprecation to actually exit in a semver major change (for node10).

[benjamingr]

I'm familiar with an empty .catch on a fork to suppress the warning, that capability is there intentionally (we considered it when we designed the hook behavior and even added a "shorthand" in bluebird, it breaks with cancellation but that's hardly a concern anymore).

I don't think it's reasonable to expect users to do p.catch(() => {}) every time they need this, or to understand they need to do this (and why). async/await is great in helping users forget about promise internals - util.promisify means they never have to write the word Promise in their code (except for .all and .race).

Both approaches (GC and nextTick) have issues:

• GC has false negatives for retention
• nextTick has false positives for things that add .catch later, which is more of an issue with async/await than before.

Which is why I think we should warn (with a friendlier warning and a trace) on nextTick and exit the process on GC - so we exit safely and warn safely.

[MadaraUchiha]

On it. #goodnessSquad

[ilkinsoft]

In my case, the reason was app-preferences plugin in Ionic 3 project.
So, I was getting this error:

_(node:21236) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Unexpected end
Line: 0
Column: 0
Char:
(node:21236) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code._

And couldn't remove plugin by ionic cordova plugin remove cordova-plugin-app-preferences command (same error was occuring again). I solved the problem by following these steps:

1. Run npm uninstall --save @ionic-native/app-preferences.
2. Opened [project_name]/config.xml file and removed <plugin name="cordova-plugin-app-preferences" spec="~0.99.3" /> tag.
3. Opened [project_name]/package.json file and removed "cordova-plugin-app-preferences": {} under "cordova" and then "plugins".
4. Went to [project_name]/plugins/ folder and removed cordova-plugin-app-preferences folder.
5. Opened [project_name]/plugins/android.json file and removed

"cordova-plugin-app-preferences": {
      "PACKAGE_NAME": "io.ionic.starter"
    }

under installed_plugins.

1. Opened [project_name]/plugins/fetch.json file and removed

  "cordova-plugin-app-preferences": {
    "source": {
      "type": "registry",
      "id": "cordova-plugin-app-preferences"
    },
    "is_top_level": true,
    "variables": {}
  }

Note that, the app-preferences plugin isn't working properly with cordova 8.0.0.

[subhashekkaluru]

D:ANGUALRJS2 PROJECTSPR02>ngh
An error occurred!

(node:4020) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): Error: Unspecified error (run without silent option for detail)
(node:4020) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

[diatoz]

I am also with stuck with this error during build. I am using node v10.0.0
(node:19949) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1) (node:19949) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

[benjamingr]

Did you not get a stack trace with that? Is that the full error @diatoz @subhashekkaluru ?

[lednax]

I also have the same issue. Nothing on the stack trace.

(node:15362) UnhandledPromiseRejectionWarning: #<Object>
    at emitWarning (internal/process/promises.js:67:17)
    at emitPendingUnhandledRejections (internal/process/promises.js:109:11)
    at runMicrotasksCallback (internal/process/next_tick.js:124:9)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickDomainCallback (internal/process/next_tick.js:218:9)
(node:15362) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 3)
    at emitWarning (internal/process/promises.js:75:21)
    at emitPendingUnhandledRejections (internal/process/promises.js:109:11)
    at runMicrotasksCallback (internal/process/next_tick.js:124:9)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickDomainCallback (internal/process/next_tick.js:218:9)
(node:15362) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
    at emitWarning (internal/process/promises.js:92:15)
    at emitPendingUnhandledRejections (internal/process/promises.js:109:11)
    at runMicrotasksCallback (internal/process/next_tick.js:124:9)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickDomainCallback (internal/process/next_tick.js:218:9)

node -v v8.11.1
npm 6.0.1

[lucasdellabella]

@benjamingr running with --trace-warnings was enough to help me solve my own problems. Maybe just add a suggestion to try to using the --trace-warnings flag to the error message as a temporary improvement? I was really lost until I could at least see the trace.

thanks for the explanation

[benjamingr]

@lucasdellabella thanks for commenting :) Could always use more people interested in improving the debugging experience in Node.js.

We've made the stack trace always show (even without the --trace-warnings flag) in Node.js 10 - so a possible solution would be to update your Node.js version.

The V8 team are working on giving us async stack traces in production - so we'll get much better errors soon too :)

[Robiv8]

I have this error in pm2 logs mm

`

0|mm | (node:5263) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): TypeError: Cannot read property 'refs' of undefined
0|mm | (node:5263) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
0|mm | (node:5263) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): TypeError: Cannot read property 'refs' of undefined

`

[j3pic]

Hopefully this new feature doesn't cause promise rejections to kill a REPL session. I sometimes do things like this:

> var p = some_async_function_that_might_fail();

....and I see the UnhandledPromiseRejectionWarning immediately. I shouldn't have to type in a .catch() handler with every REPL statement. That comes close to defeating the whole purpose of a REPL.

[Fr33maan]

Is there any flag to make this deprecation a reality ? I would like to avoid having unhandled promises not returning a non 0 exit code.
Typically, I have an async function at the top level and right now the only way I find to actually crash is to process.on('unhandledRejection', up => { throw up }) somewhere in my code

[tniessen]

@l1br3 --unhandled-rejections=strict?

[Fr33maan]

@tniessen well tried :-) but gave me this error: /home/libre/.nvm/versions/node/v11.2.0/bin/node: bad option: --unhandled-rejections=strict error Command failed with exit code 9.

EDIT: working correctly on node v12.x

[benjamingr]

@l1br3 you are running an old version of Node

[Fr33maan]

@benjamingr Well spotted ! That was the issue, the flag is working correctly. Thanks !