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

process: unhandledRejection support more errors #41682

Merged
merged 2 commits into from
Jan 28, 2022
Merged

process: unhandledRejection support more errors #41682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f493e9
process: unhandledRejection support more errors
benjamingr Jan 24, 2022
9a234a5
add test
benjamingr Jan 25, 2022
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
22 changes: 18 additions & 4 deletions lib/internal/process/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const {
ArrayPrototypeShift,
Error,
ObjectDefineProperty,
ObjectPrototypeHasOwnProperty,
SafeWeakMap,
} = primordials;

Expand Down Expand Up @@ -79,6 +80,12 @@ function hasRejectionToWarn() {
return tickInfo[kHasRejectionToWarn] === 1;
}

function isErrorLike(o) {
return typeof o === 'object' &&
o !== null &&
ObjectPrototypeHasOwnProperty(o, 'stack');
Copy link
Contributor

@bnb bnb Jan 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

might want to use Object.hasOwn() :)

Copy link
Member Author

@benjamingr benjamingr Jan 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@bnb I wanted to but then we can't backport it to older versions of Node - see discussion here: #41682 (comment)

}

function getUnhandledRejectionsMode() {
const { getOptionValue } = require('internal/options');
switch (getOptionValue('--unhandled-rejections')) {
Expand Down Expand Up @@ -179,14 +186,21 @@ function emitUnhandledRejectionWarning(uid, reason) {
`(rejection id: ${uid})`
);
try {
if (reason instanceof Error) {
if (isErrorLike(reason)) {
warning.stack = reason.stack;
process.emitWarning(reason.stack, unhandledRejectionErrName);
} else {
process.emitWarning(
noSideEffectsToString(reason), unhandledRejectionErrName);
}
} catch {}
} catch {
try {
process.emitWarning(
noSideEffectsToString(reason), unhandledRejectionErrName);
} catch {
// Ignore.
}
}

process.emitWarning(warning);
}
Expand Down Expand Up @@ -232,7 +246,7 @@ function processPromiseRejections() {
try {
switch (unhandledRejectionsMode) {
case kStrictUnhandledRejections: {
const err = reason instanceof Error ?
const err = isErrorLike(reason) ?
reason : generateUnhandledRejectionError(reason);
// This destroys the async stack, don't clear it after
triggerUncaughtException(err, true /* fromPromise */);
Expand All @@ -259,7 +273,7 @@ function processPromiseRejections() {
case kThrowUnhandledRejections: {
const handled = emit(reason, promise, promiseInfo);
if (!handled) {
const err = reason instanceof Error ?
const err = isErrorLike(reason) ?
reason : generateUnhandledRejectionError(reason);
// This destroys the async stack, don't clear it after
triggerUncaughtException(err, true /* fromPromise */);
Expand Down
19 changes: 17 additions & 2 deletions test/parallel/test-promise-unhandled-warn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
'use strict';

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

// Verify that ignoring unhandled rejection works fine and that no warning is
// logged.
Expand All @@ -12,11 +13,25 @@ new Promise(() => {

Promise.reject('test');

function lookForMeInStackTrace() {
Promise.reject(new class ErrorLike {
constructor() {
Error.captureStackTrace(this);
this.message = 'ErrorLike';
}
}());
}
lookForMeInStackTrace();

// Unhandled rejections trigger two warning per rejection. One is the rejection
// reason and the other is a note where this warning is coming from.
process.on('warning', common.mustCall(4));
process.on('warning', common.mustCall((reason) => {
Copy link

@ItamarGronich ItamarGronich Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What will happen if --unhandled-rejections=throw (which is the default nowadays)
does this test cover it? the title says unhandled-warn so maybe not this test, maybe need to add to modify another test which handles the throw value?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ItamarGronich there is a separate test for test-promise-unhandled-throw though that's harder to test given it's process output.

if (reason.message.includes('ErrorLike')) {
assert.match(reason.stack, /lookForMeInStackTrace/);
}
}, 6));
process.on('uncaughtException', common.mustNotCall('uncaughtException'));
process.on('rejectionHandled', common.mustCall(2));
process.on('rejectionHandled', common.mustCall(3));

process.on('unhandledRejection', (reason, promise) => {
// Handle promises but still warn!
Expand Down