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

process: Add --unhandled-rejections=throw and =warn-with-error-code #33475

Closed
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
6 changes: 5 additions & 1 deletion doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -971,11 +971,15 @@ for the very first unhandled rejection in case no [`unhandledRejection`][] hook
is used.

Using this flag allows to change what should happen when an unhandled rejection
occurs. One of three modes can be chosen:
occurs. One of the following modes can be chosen:

* `throw`: Emit [`unhandledRejection`][]. If this hook is not set, raise the
unhandled rejection as an uncaught exception.
* `strict`: Raise the unhandled rejection as an uncaught exception.
* `warn`: Always trigger a warning, no matter if the [`unhandledRejection`][]
hook is set or not but do not print the deprecation warning.
* `warn-with-error-code`: Emit [`unhandledRejection`][]. If this hook is not
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for bikeshedding the name further but what about: warn-and-exit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm OK with a bit more bikeshedding on this, but be advised that I'd already filed a PR to update the name in the unhandled rejections survey. nodejs/TSC#872 It

IMO, warn-and-exit sounds like it will actually terminate the process, but this flag is designed to set process.exitCode = 1 and keep running. The exit code may never even take effect if it's a long-running process that never terminates until killed.

I think warn-with-error-code is the shortest and clearest way to say, "trigger a warning, and set the process exit code to 1"

Copy link
Contributor

Choose a reason for hiding this comment

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

@dfabulich I don't think it's a problem if we end up with different names on the survey, because we explain what each mode is there (and we're only using mode names there as an alias for what they mean).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For my part, I can’t think of a better name for this than warn-with-error-code. Let’s merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it time?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's ready to land. I'm very absent on GitHub due to everything that is happening, but another collaborator might land it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it help if this PR had the "author ready" label?

set, trigger a warning, and set the process exit code to 1.
* `none`: Silence all warnings.

### `--use-bundled-ca`, `--use-openssl-ca`
Expand Down
52 changes: 44 additions & 8 deletions lib/internal/process/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,37 @@ const pendingUnhandledRejections = [];
const asyncHandledRejections = [];
let lastPromiseId = 0;

// --unhandled-rejection=none:
// --unhandled-rejections=none:
// Emit 'unhandledRejection', but do not emit any warning.
const kIgnoreUnhandledRejections = 0;
// --unhandled-rejection=warn:

// --unhandled-rejections=warn:
// Emit 'unhandledRejection', then emit 'UnhandledPromiseRejectionWarning'.
const kAlwaysWarnUnhandledRejections = 1;
// --unhandled-rejection=strict:

// --unhandled-rejections=strict:
// Emit 'uncaughtException'. If it's not handled, print the error to stderr
// and exit the process.
// Otherwise, emit 'unhandledRejection'. If 'unhandledRejection' is not
// handled, emit 'UnhandledPromiseRejectionWarning'.
const kThrowUnhandledRejections = 2;
// --unhandled-rejection is unset:
// Emit 'unhandledRejection', if it's handled, emit
BridgeAR marked this conversation as resolved.
Show resolved Hide resolved
const kStrictUnhandledRejections = 2;

// --unhandled-rejections=throw:
// Emit 'unhandledRejection', if it's unhandled, emit
// 'uncaughtException'. If it's not handled, print the error to stderr
// and exit the process.
const kThrowUnhandledRejections = 3;

// --unhandled-rejections=warn-with-error-code:
// Emit 'unhandledRejection', if it's unhandled, emit
// 'UnhandledPromiseRejectionWarning', then set process exit code to 1.

const kWarnWithErrorCodeUnhandledRejections = 4;

// --unhandled-rejections is unset:
// Emit 'unhandledRejection', if it's unhandled, emit
// 'UnhandledPromiseRejectionWarning', then emit deprecation warning.
const kDefaultUnhandledRejections = 3;
const kDefaultUnhandledRejections = 5;
dfabulich marked this conversation as resolved.
Show resolved Hide resolved

let unhandledRejectionsMode;

Expand All @@ -65,7 +80,11 @@ function getUnhandledRejectionsMode() {
case 'warn':
return kAlwaysWarnUnhandledRejections;
case 'strict':
return kStrictUnhandledRejections;
case 'throw':
return kThrowUnhandledRejections;
case 'warn-with-error-code':
return kWarnWithErrorCodeUnhandledRejections;
default:
return kDefaultUnhandledRejections;
}
Expand Down Expand Up @@ -188,7 +207,7 @@ function processPromiseRejections() {
promiseInfo.warned = true;
const { reason, uid } = promiseInfo;
switch (unhandledRejectionsMode) {
case kThrowUnhandledRejections: {
case kStrictUnhandledRejections: {
const err = reason instanceof Error ?
reason : generateUnhandledRejectionError(reason);
triggerUncaughtException(err, true /* fromPromise */);
Expand All @@ -205,6 +224,23 @@ function processPromiseRejections() {
emitUnhandledRejectionWarning(uid, reason);
break;
}
case kThrowUnhandledRejections: {
const handled = process.emit('unhandledRejection', reason, promise);
if (!handled) {
const err = reason instanceof Error ?
dfabulich marked this conversation as resolved.
Show resolved Hide resolved
reason : generateUnhandledRejectionError(reason);
triggerUncaughtException(err, true /* fromPromise */);
}
break;
}
case kWarnWithErrorCodeUnhandledRejections: {
const handled = process.emit('unhandledRejection', reason, promise);
if (!handled) {
emitUnhandledRejectionWarning(uid, reason);
process.exitCode = 1;
}
break;
}
case kDefaultUnhandledRejections: {
const handled = process.emit('unhandledRejection', reason, promise);
if (!handled) {
Expand Down
2 changes: 2 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
}

if (!unhandled_rejections.empty() &&
unhandled_rejections != "warn-with-error-code" &&
unhandled_rejections != "throw" &&
unhandled_rejections != "strict" &&
unhandled_rejections != "warn" &&
unhandled_rejections != "none") {
Expand Down
10 changes: 10 additions & 0 deletions test/message/promise_unhandled_warn_with_error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Flags: --unhandled-rejections=warn-with-error-code
'use strict';

const common = require('../common');
dfabulich marked this conversation as resolved.
Show resolved Hide resolved
const assert = require('assert');

common.disableCrashOnUnhandledRejection();

Promise.reject(new Error('alas'));
dfabulich marked this conversation as resolved.
Show resolved Hide resolved
process.on('exit', assert.strictEqual.bind(null, 1));
10 changes: 10 additions & 0 deletions test/message/promise_unhandled_warn_with_error.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
*UnhandledPromiseRejectionWarning: Error: alas
at *promise_unhandled_warn_with_error.js:*:*
at *
at *
at *
at *
at *
at *
(Use `node --trace-warnings ...` to show where the warning was created)
*UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
38 changes: 38 additions & 0 deletions test/parallel/test-promise-unhandled-throw-handler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Flags: --unhandled-rejections=throw
'use strict';

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

common.disableCrashOnUnhandledRejection();

// Verify that the unhandledRejection handler prevents triggering
// uncaught exceptions

const err1 = new Error('One');

const errors = [err1, null];

const ref = new Promise(() => {
throw err1;
});
// Explicitly reject `null`.
Promise.reject(null);

process.on('warning', common.mustNotCall('warning'));
process.on('rejectionHandled', common.mustNotCall('rejectionHandled'));
process.on('exit', assert.strictEqual.bind(null, 0));
process.on('uncaughtException', common.mustNotCall('uncaughtException'));

const timer = setTimeout(() => console.log(ref), 1000);

const counter = new Countdown(2, () => {
clearTimeout(timer);
});

process.on('unhandledRejection', common.mustCall((err) => {
counter.dec();
const knownError = errors.shift();
assert.deepStrictEqual(err, knownError);
}, 2));
55 changes: 55 additions & 0 deletions test/parallel/test-promise-unhandled-throw.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Flags: --unhandled-rejections=throw
'use strict';

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

common.disableCrashOnUnhandledRejection();

// Verify that unhandled rejections always trigger uncaught exceptions instead
// of triggering unhandled rejections.

const err1 = new Error('One');
const err2 = new Error(
'This error originated either by throwing ' +
'inside of an async function without a catch block, or by rejecting a ' +
'promise which was not handled with .catch(). The promise rejected with the' +
' reason "null".'
);
err2.code = 'ERR_UNHANDLED_REJECTION';
Object.defineProperty(err2, 'name', {
value: 'UnhandledPromiseRejection',
writable: true,
configurable: true
});

const errors = [err1, err2];
const identical = [true, false];

const ref = new Promise(() => {
throw err1;
});
// Explicitly reject `null`.
Promise.reject(null);

process.on('warning', common.mustNotCall('warning'));
// If we add an unhandledRejection handler, the exception won't be thrown
// process.on('unhandledRejection', common.mustCall(2));
dfabulich marked this conversation as resolved.
Show resolved Hide resolved
process.on('rejectionHandled', common.mustNotCall('rejectionHandled'));
process.on('exit', assert.strictEqual.bind(null, 0));

const timer = setTimeout(() => console.log(ref), 1000);

const counter = new Countdown(2, () => {
clearTimeout(timer);
});

process.on('uncaughtException', common.mustCall((err, origin) => {
counter.dec();
assert.strictEqual(origin, 'unhandledRejection', err);
const knownError = errors.shift();
assert.deepStrictEqual(err, knownError);
// Check if the errors are reference equal.
assert(identical.shift() ? err === knownError : err !== knownError);
}, 2));