From 96a9d530a938ea9b50c111757c4fbb2598b4985d Mon Sep 17 00:00:00 2001 From: Benjamin Gruenbaum Date: Thu, 28 Apr 2016 11:11:09 +0300 Subject: [PATCH 1/2] 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. --- lib/internal/process/promises.js | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index 22f1959784be9a..dcc0dc75d38c66 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -2,6 +2,8 @@ const promiseRejectEvent = process._promiseRejectEvent; const hasBeenNotifiedProperty = new WeakMap(); +const promiseToGuidProperty = new WeakMap(); +var lastPromiseId = 1; const pendingUnhandledRejections = []; exports.setup = setupPromises; @@ -18,6 +20,7 @@ function setupPromises(scheduleMicrotasks) { function unhandledRejection(promise, reason) { hasBeenNotifiedProperty.set(promise, false); + promiseToGuidProperty.set(promise, lastPromiseId++); addPendingUnhandledRejection(promise, reason); } @@ -25,9 +28,15 @@ function setupPromises(scheduleMicrotasks) { 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)) { + process.emitWarning('Possibly Unhandled Promise Rejection Handled ' + + '(unhandled rejection id: ' + uid +') ', + 'PromiseRejectionHandledWarning'); + } }); } @@ -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); if (!process.emit('unhandledRejection', reason, promise)) { - // Nobody is listening. - // TODO(petkaantonov) Take some default action, see #830 + process.emitWarning('Possibly unhandled promise rejection ' + + '(unhandled rejection id: ' + uid +'): ' + + reason, + 'UnhandledPromiseRejectionWarning'); } else { hadListeners = true; } From cbe67802f06a501dd480fd7aa57d2bb439f4bcf0 Mon Sep 17 00:00:00 2001 From: Benjamin Gruenbaum Date: Sun, 1 May 2016 19:02:48 +0300 Subject: [PATCH 2/2] address feedback --- lib/internal/process/promises.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index dcc0dc75d38c66..563517c94e2936 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -3,8 +3,8 @@ const promiseRejectEvent = process._promiseRejectEvent; const hasBeenNotifiedProperty = new WeakMap(); const promiseToGuidProperty = new WeakMap(); -var lastPromiseId = 1; const pendingUnhandledRejections = []; +let lastPromiseId = 1; exports.setup = setupPromises; @@ -33,8 +33,8 @@ function setupPromises(scheduleMicrotasks) { if (hasBeenNotified === true) { process.nextTick(function() { if(!process.emit('rejectionHandled', promise)) { - process.emitWarning('Possibly Unhandled Promise Rejection Handled ' - + '(unhandled rejection id: ' + uid +') ', + process.emitWarning('Promise Rejection Handled Asynchronously' + + '(rejection id: ' + uid +') ', 'PromiseRejectionHandledWarning'); } }); @@ -44,16 +44,16 @@ function setupPromises(scheduleMicrotasks) { } function emitPendingUnhandledRejections() { - var hadListeners = false; + let hadListeners = false; while (pendingUnhandledRejections.length > 0) { - var promise = pendingUnhandledRejections.shift(); - var reason = pendingUnhandledRejections.shift(); + const promise = pendingUnhandledRejections.shift(); + const reason = pendingUnhandledRejections.shift(); if (hasBeenNotifiedProperty.get(promise) === false) { hasBeenNotifiedProperty.set(promise, true); const uid = promiseToGuidProperty.get(promise); if (!process.emit('unhandledRejection', reason, promise)) { process.emitWarning('Possibly unhandled promise rejection ' - + '(unhandled rejection id: ' + uid +'): ' + + '(rejection id: ' + uid +'): ' + reason, 'UnhandledPromiseRejectionWarning'); } else {