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.

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

Jump to bottom

promise: warn on unhandled rejections #6439

Closed
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
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

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
Contributor

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
Member

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
Member Author

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

@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
Contributor

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
Member Author

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
Member

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
Contributor

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
Member Author

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
Contributor

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