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.

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

promise: warning on unhandled rejection #8217

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 30 additions & 6 deletions lib/internal/process/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

const promiseRejectEvent = process._promiseRejectEvent;
const hasBeenNotifiedProperty = new WeakMap();
const promiseToGuidProperty = new WeakMap();
const pendingUnhandledRejections = [];
let lastPromiseId = 1;

exports.setup = setupPromises;

Expand All @@ -18,32 +20,54 @@ 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move this variable to define at line 36?

promiseToGuidProperty.delete(promise);
if (hasBeenNotified === true) {
process.nextTick(function() {
process.emit('rejectionHandled', promise);
if (!process.emit('rejectionHandled', promise)) {
const warning = new Error('Promise rejection was handled ' +
`asynchronously (rejection id: ${uid})`);
warning.name = 'PromiseRejectionHandledWarning';
warning.id = uid;
process.emitWarning(warning);
}
});
}

}
}

var deprecationWarned = false;
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)) {
// Nobody is listening.
// TODO(petkaantonov) Take some default action, see #830
const warning = new Error('Unhandled promise rejection ' +
`(rejection id: ${uid}): ${reason}`);
warning.name = 'UnhandledPromiseRejectionWarning';
warning.id = uid;
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');
}
} else {
hadListeners = true;
}
Expand Down
29 changes: 29 additions & 0 deletions test/parallel/test-promises-warning-on-unhandled-rejection.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Flags: --no-warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

should be unnecessary since we are handling it?

Copy link
Member Author

Choose a reason for hiding this comment

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

This prevents the unnecessary stderr output so the test isn't needlessly noisy.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasnell It should never reach stderr since we are listening to it. is there a bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not how emitWarning works. By default it prints to stderr and emits the 'warning' event. The --no-warnings switch suppresses the default stderr output but maintains the 'warning' event. There is no bug.

'use strict';

// Test that warnings are emitted when a Promise experiences an uncaught
// rejection, and then again if the rejection is handled later on.

const common = require('../common');
const assert = require('assert');

var b = 0;

process.on('warning', common.mustCall((warning) => {
switch (b++) {
case 0:
assert.strictEqual(warning.name, 'UnhandledPromiseRejectionWarning');
assert(/Unhandled promise rejection/.test(warning.message));
break;
case 1:
assert.strictEqual(warning.name, 'DeprecationWarning');
break;
case 2:
assert.strictEqual(warning.name, 'PromiseRejectionHandledWarning');
assert(/Promise rejection was handled asynchronously/
.test(warning.message));
}
}, 3));

const p = Promise.reject('This was rejected');
setImmediate(common.mustCall(() => p.catch(() => {})));