Skip to content

promise: warn on unhandled rejections #6439

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.

Sign up for GitHub

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

Closed
benjamingr wants to merge 2 commits into from
Closed

promise: warn on unhandled rejections #6439

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96a9d53
promise: warn on unhandled rejections
benjamingr Apr 28, 2016
cbe6780
address feedback
benjamingr May 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
promise: warn on unhandled rejections 
I think that we all _agree_ that the current default behavior in node for unhandled rejections is very wrong, people running code that behaved well in browsers suddenly get their exceptions swallowed by default and have to read the process docs and dig up an API. Programmer errors are swallowed by default and are very hard to find.

This makes NodeJS a lot less approachable to promise users* which are slowly but surely becoming a majority as web APIs switch to promises and a lot already have. 


This pull request suggests 
 - We log the exception with a GUID when `unhandledRejection` happens and the text "Warning: Possibly unhandled rejection in ...". It is treated as a _warning_ and not an error - we use the new `process.warning` API that @jasnell has suggested so it is treated like a warning and not an error.
 - We log a "Possibly unhandled rejection eventually handled" message (also, as a warning) when that error is taken care of.

This approach has the following advantages:
 - Most backwards compatible with the current behavior while solving the issue for 99% of users.
 - This takes care of the "always right" issue as it's a warning and there might be valid use cases for not handling it. Kind of like the very helpful EventEmitter # of subscribers warning.
 - Users hooking on warnings have this dealt with like other warnings, they can suppress it in production, deal with it or do anything else.
 - Users installing custom handlers have nothing broken in their code and everything proceeds as normal.
 - Easy to override this behavior either at the promise or warning level.
  • Loading branch information
@benjamingr
benjamingr committed Apr 28, 2016
commit 96a9d530a938ea9b50c111757c4fbb2598b4985d
18 changes: 15 additions & 3 deletions lib/internal/process/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

const promiseRejectEvent = process._promiseRejectEvent;
const hasBeenNotifiedProperty = new WeakMap();
const promiseToGuidProperty = new WeakMap();
var lastPromiseId = 1;
Copy link
Copy Markdown

@sagivo sagivo Apr 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe let ?

const pendingUnhandledRejections = [];

exports.setup = setupPromises;
Expand All @@ -18,16 +20,23 @@ function setupPromises(scheduleMicrotasks) {

function unhandledRejection(promise, reason) {
hasBeenNotifiedProperty.set(promise, false);
promiseToGuidProperty.set(promise, lastPromiseId++);
addPendingUnhandledRejection(promise, reason);
}

function rejectionHandled(promise) {
var hasBeenNotified = hasBeenNotifiedProperty.get(promise);
if (hasBeenNotified !== undefined) {
hasBeenNotifiedProperty.delete(promise);
const uid = promiseToGuidProperty.get(promise);
promiseToGuidProperty.delete(promise);
if (hasBeenNotified === true) {
process.nextTick(function() {
process.emit('rejectionHandled', promise);
if(!process.emit('rejectionHandled', promise)) {
Copy link
Copy Markdown
Contributor

@Fishrock123 Fishrock123 May 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (

process.emitWarning('Possibly Unhandled Promise Rejection Handled '
Copy link
Copy Markdown
Member

@jasnell jasnell Apr 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message Possibly Unhandled Promise Rejection Handled is a bit awkward. How can it be a possibly unhandled rejection if it was handled? ;-)

Copy link
Copy Markdown
Member Author

@benjamingr benjamingr Apr 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's all confusing terminology, I'm very open to different naming - the PR is a strawman to discuss this sort of thing exactly.

Maybe 'Previous reported promise rejection handled'?

+ '(unhandled rejection id: ' + uid +') ',
'PromiseRejectionHandledWarning');
}
});
}

Expand All @@ -41,9 +50,12 @@ function setupPromises(scheduleMicrotasks) {
var reason = pendingUnhandledRejections.shift();
if (hasBeenNotifiedProperty.get(promise) === false) {
hasBeenNotifiedProperty.set(promise, true);
const uid = promiseToGuidProperty.get(promise);
Copy link
Copy Markdown

@sagivo sagivo Apr 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

promiseToGuidProperty.get(promise) uses also here, worth making it a method instead and hiding promiseToGuidProperty from the rest of the world?

if (!process.emit('unhandledRejection', reason, promise)) {
// Nobody is listening.
// TODO(petkaantonov) Take some default action, see #830
process.emitWarning('Possibly unhandled promise rejection '
Copy link
Copy Markdown
Contributor

@Fishrock123 Fishrock123 Apr 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should double emit. Why not just print it, or use the internal warning logic directly?

I also definitely don't think this should be "catchable" in the warning event handler. People should use unhandledRejection directly for these, whatever our final conclusion is.

Copy link
Copy Markdown
Member Author

@benjamingr benjamingr Apr 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the point of this PR was to use @jasnell's idea of treating them as warnings.

If we consider them warning and not errors initially I see double benefit:

A) It's theoretically sound, since we're not claiming it's an error but a warning (like adding a lot of listeners to an EventEmitter).
B) It's easy to turn off in production or log cleanly using whatever infrastructure people use for warnings.

Note that rejections are pretty rare (like actual exceptions) and unhandled rejections even more so - so the performance penalty here is negligible.

By emitting a warning rather than directly printing an error - I think we're leaving a much bigger opportunity for a long term solution: people realize warnings might go away, can be turned off, can be reported to another channel than errors and so on.

This does not contradict a "throw on unhandled GC" or another "long term" solution. It does solve the problem of promises being terribly hard for users to debug with the current default which is what people always complain about in StackOverflow and forums.

I think it's a reasonable compromise and that we haven't been fair to our users since we landed 'unhandledRejection'. I take personal blame for not pushing harder on a reasonable solution for users for over a year.

Copy link
Copy Markdown
Member

@jasnell jasnell Apr 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could potentially check the listener count for unhandledRejection first and only emit if it's greater than zero, otherwise emit the warning.

This is precisely the kind of thing emitWarning is for. -1 for printing directly.

Copy link
Copy Markdown
Contributor

@chrisdickinson chrisdickinson Apr 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with emitWarning here — the value in double emitting is that, as a user of a system that is designed around NDJSON-based logs, I can easily hook into the warning system and turn this message into a JSON log (vs. having to rely on patching out console.)

I'd quibble that we should probably log this as an Unhandled promise rejection, versus "possibly unhandled promise rejection." At the moment of logging, the promise is unhandled, and we cannot say whether or not it will be handled in the future. If it is possible for the rejection to become handled, that case will be caught by the "rejection handled" warning.

Copy link
Copy Markdown
Member Author

@benjamingr benjamingr Apr 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell what you are describing is exactly the behavior in this PR, emitWarning is only called if there are no unhandledRejection listeners :)

Copy link
Copy Markdown
Contributor

@Fishrock123 Fishrock123 Apr 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrisdickinson's point makes sense.

+ '(unhandled rejection id: ' + uid +'): '
+ reason,
'UnhandledPromiseRejectionWarning');
} else {
hadListeners = true;
}
Expand Down