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
   

   node/lib/internal/process/promises.js

   Lines 62 to 84 in 3a4f0e9

   	function emitWarning(uid, reason) {
   	const warning = new Error(
   	`Unhandled promise rejection (rejection id: ${uid}): ` +
   	safeToString(reason));
   	warning.name = 'UnhandledPromiseRejectionWarning';
   	warning.id = uid;
   	try {
   	if (reason instanceof Error) {
   	warning.stack = reason.stack;
   	}
   	} catch (err) {
   	// ignored
   	}
   	process.emitWarning(warning);
   	if (!deprecationWarned) {
   	deprecationWarned = true;
   	process.emitWarning(
   	'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.',
   	'DeprecationWarning', 'DEP0018');
   	}
   	}

[benjamingr]

#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.