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 flag #26599

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
19 changes: 19 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,23 @@ added: v2.4.0

Track heap object allocations for heap snapshots.

### `--unhandled-rejections=mode`
<!-- YAML
added: REPLACEME
-->

By default all unhandled rejections trigger a warning plus a deprecation warning
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:

* `strict`: Raise the unhandled rejection as an uncaught exception.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be error?

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally planned on using error and silence instead of strict and none but others preferred these and I have no strong opinion on the name in this case.

Copy link
Member

Choose a reason for hiding this comment

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

nit: may want to add a quick statement that the mode is case-sensitive.

* `warn`: Always trigger a warning, no matter if the [`unhandledRejection`][]
hook is set or not but do not print the deprecation warning.
* `none`: Silence all warnings.

### `--use-bundled-ca`, `--use-openssl-ca`
<!-- YAML
added: v6.11.0
Expand Down Expand Up @@ -789,6 +806,7 @@ Node.js options that are allowed are:
- `--trace-sync-io`
- `--trace-warnings`
- `--track-heap-objects`
- `--unhandled-rejections`
- `--use-bundled-ca`
- `--use-openssl-ca`
- `--v8-pool-size`
Expand Down Expand Up @@ -957,6 +975,7 @@ greater than `4` (its current default value). For more information, see the
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[`tls.DEFAULT_MAX_VERSION`]: tls.html#tls_tls_default_max_version
[`tls.DEFAULT_MIN_VERSION`]: tls.html#tls_tls_default_min_version
[`unhandledRejection`]: process.html#process_event_unhandledrejection
[Chrome DevTools Protocol]: https://chromedevtools.github.io/devtools-protocol/
[REPL]: repl.html
[ScriptCoverage]: https://chromedevtools.github.io/devtools-protocol/tot/Profiler#type-ScriptCoverage
Expand Down
37 changes: 22 additions & 15 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,17 @@ most convenient for scripts).
### Event: 'uncaughtException'
<!-- YAML
added: v0.1.18
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/REPLACEME
description: Added the `origin` argument.
-->

* `err` {Error} The uncaught exception.
* `origin` {string} Indicates if the exception originates from an unhandled
rejection or from synchronous errors. Can either be `'uncaughtException'` or
`'unhandledRejection'`.

The `'uncaughtException'` event is emitted when an uncaught JavaScript
exception bubbles all the way back to the event loop. By default, Node.js
handles such exceptions by printing the stack trace to `stderr` and exiting
Expand All @@ -217,12 +226,13 @@ behavior. Alternatively, change the [`process.exitCode`][] in the
provided exit code. Otherwise, in the presence of such handler the process will
exit with 0.

The listener function is called with the `Error` object passed as the only
argument.

```js
process.on('uncaughtException', (err) => {
fs.writeSync(1, `Caught exception: ${err}\n`);
process.on('uncaughtException', (err, origin) => {
fs.writeSync(
process.stderr.fd,
`Caught exception: ${err}\n` +
`Exception origin: ${origin}`
);
});

setTimeout(() => {
Expand Down Expand Up @@ -274,6 +284,10 @@ changes:
a process warning.
-->

* `reason` {Error|any} The object with which the promise was rejected
(typically an [`Error`][] object).
* `promise` {Promise} The rejected promise.

The `'unhandledRejection'` event is emitted whenever a `Promise` is rejected and
no error handler is attached to the promise within a turn of the event loop.
When programming with Promises, exceptions are encapsulated as "rejected
Expand All @@ -282,15 +296,9 @@ are propagated through a `Promise` chain. The `'unhandledRejection'` event is
useful for detecting and keeping track of promises that were rejected whose
rejections have not yet been handled.

The listener function is called with the following arguments:

* `reason` {Error|any} The object with which the promise was rejected
(typically an [`Error`][] object).
* `p` the `Promise` that was rejected.

```js
process.on('unhandledRejection', (reason, p) => {
console.log('Unhandled Rejection at:', p, 'reason:', reason);
process.on('unhandledRejection', (reason, promise) => {
console.log('Unhandled Rejection at:', promise, 'reason:', reason);
// Application specific logging, throwing an error, or other logic here
});

Expand All @@ -317,7 +325,7 @@ as would typically be the case for other `'unhandledRejection'` events. To
address such failures, a non-operational
[`.catch(() => { })`][`promise.catch()`] handler may be attached to
`resource.loaded`, which would prevent the `'unhandledRejection'` event from
being emitted. Alternatively, the [`'rejectionHandled'`][] event may be used.
being emitted.

### Event: 'warning'
<!-- YAML
Expand Down Expand Up @@ -2282,7 +2290,6 @@ cases:

[`'exit'`]: #process_event_exit
[`'message'`]: child_process.html#child_process_event_message
[`'rejectionHandled'`]: #process_event_rejectionhandled
[`'uncaughtException'`]: #process_event_uncaughtexception
[`ChildProcess.disconnect()`]: child_process.html#child_process_subprocess_disconnect
[`ChildProcess.send()`]: child_process.html#child_process_subprocess_send_message_sendhandle_options_callback
Expand Down
3 changes: 3 additions & 0 deletions doc/node.1
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,9 @@ Print stack traces for process warnings (including deprecations).
.It Fl -track-heap-objects
Track heap object allocations for heap snapshots.
.
.It Fl --unhandled-rejections=mode
Define the behavior for unhandled rejections. Can be one of `strict` (raise an error), `warn` (enforce warnings) or `none` (silence warnings).
.
.It Fl -use-bundled-ca , Fl -use-openssl-ca
Use bundled Mozilla CA store as supplied by current Node.js version or use OpenSSL's default CA store.
The default store is selectable at build-time.
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/main/worker_thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,11 @@ port.on('message', (message) => {
});

// Overwrite fatalException
process._fatalException = (error) => {
process._fatalException = (error, fromPromise) => {
debug(`[${threadId}] gets fatal exception`);
let caught = false;
try {
caught = originalFatalException.call(this, error);
caught = originalFatalException.call(this, error, fromPromise);
} catch (e) {
error = e;
}
Expand Down
5 changes: 4 additions & 1 deletion lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,10 @@ Module.runMain = function() {
return loader.import(pathToFileURL(process.argv[1]).pathname);
})
.catch((e) => {
internalBinding('task_queue').triggerFatalException(e);
internalBinding('task_queue').triggerFatalException(
e,
true /* fromPromise */
);
});
// Handle any nextTicks added in the first tick of the program
process._tickCallback();
Expand Down
5 changes: 3 additions & 2 deletions lib/internal/process/execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ function noop() {}
// before calling into process._fatalException, or this function should
// take extra care of the async hooks before it schedules a setImmediate.
function createFatalException() {
return (er) => {
return (er, fromPromise) => {
// It's possible that defaultTriggerAsyncId was set for a constructor
// call that threw and was never cleared. So clear it now.
clearDefaultTriggerAsyncId();
Expand All @@ -138,9 +138,10 @@ function createFatalException() {
} catch {} // Ignore the exception. Diagnostic reporting is unavailable.
}

const type = fromPromise ? 'unhandledRejection' : 'uncaughtException';
if (exceptionHandlerState.captureFn !== null) {
exceptionHandlerState.captureFn(er);
} else if (!process.emit('uncaughtException', er)) {
} else if (!process.emit('uncaughtException', er, type)) {
// If someone handled it, then great. otherwise, die in C++ land
// since that means that we'll exit the process, emit the 'exit' event.
try {
Expand Down
90 changes: 74 additions & 16 deletions lib/internal/process/promises.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
'use strict';

const { safeToString } = internalBinding('util');
const {
safeToString
} = internalBinding('util');
const {
tickInfo,
promiseRejectEvents: {
Expand All @@ -9,7 +11,8 @@ const {
kPromiseResolveAfterResolved,
kPromiseRejectAfterResolved
},
setPromiseRejectCallback
setPromiseRejectCallback,
triggerFatalException
} = internalBinding('task_queue');

// *Must* match Environment::TickInfo::Fields in src/env.h.
Expand All @@ -20,6 +23,15 @@ const pendingUnhandledRejections = [];
const asyncHandledRejections = [];
let lastPromiseId = 0;

const states = {
none: 0,
Copy link
Member

Choose a reason for hiding this comment

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

minor non-blocking nit... I'd prefer if these were defined as constants

const kStateNone = 0;
const kStateWarn = 1;
const kStateStrict = 2;
const kStateDefault = 3;

And would prefer a simple switch rather than an object lookup. For super small sets like this the switch should be faster

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll open a follow-up PR for the two nits. I would like to land this as is, otherwise other's have to look at this again.

Copy link
Member Author

Choose a reason for hiding this comment

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

The switch case is only executed once, so it's not about performance.

warn: 1,
strict: 2,
default: 3
};

let state;
ChALkeR marked this conversation as resolved.
Show resolved Hide resolved

function setHasRejectionToWarn(value) {
tickInfo[kHasRejectionToWarn] = value ? 1 : 0;
}
Expand All @@ -29,6 +41,10 @@ function hasRejectionToWarn() {
}

function promiseRejectHandler(type, promise, reason) {
if (state === undefined) {
const { getOptionValue } = require('internal/options');
state = states[getOptionValue('--unhandled-rejections') || 'default'];
}
switch (type) {
case kPromiseRejectWithNoHandler:
unhandledRejection(promise, reason);
Expand Down Expand Up @@ -59,6 +75,7 @@ function unhandledRejection(promise, reason) {
uid: ++lastPromiseId,
warned: false
});
// This causes the promise to be referenced at least for one tick.
pendingUnhandledRejections.push(promise);
setHasRejectionToWarn(true);
}
Expand All @@ -85,14 +102,16 @@ function handledRejection(promise) {

const unhandledRejectionErrName = 'UnhandledPromiseRejectionWarning';
function emitWarning(uid, reason) {
// eslint-disable-next-line no-restricted-syntax
const warning = new Error(
if (state === states.none) {
return;
}
const warning = getError(
unhandledRejectionErrName,
'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(). ' +
`(rejection id: ${uid})`
'throwing inside of an async function without a catch block, ' +
'or by rejecting a promise which was not handled with .catch(). ' +
`(rejection id: ${uid})`
);
warning.name = unhandledRejectionErrName;
try {
if (reason instanceof Error) {
warning.stack = reason.stack;
Expand All @@ -108,7 +127,7 @@ function emitWarning(uid, reason) {

let deprecationWarned = false;
function emitDeprecationWarning() {
if (!deprecationWarned) {
if (state === states.default && !deprecationWarned) {
deprecationWarned = true;
process.emitWarning(
'Unhandled promise rejections are deprecated. In the future, ' +
Expand All @@ -133,18 +152,57 @@ function processPromiseRejections() {
while (len--) {
const promise = pendingUnhandledRejections.shift();
const promiseInfo = maybeUnhandledPromises.get(promise);
if (promiseInfo !== undefined) {
promiseInfo.warned = true;
const { reason, uid } = promiseInfo;
if (!process.emit('unhandledRejection', reason, promise)) {
emitWarning(uid, reason);
}
maybeScheduledTicks = true;
if (promiseInfo === undefined) {
continue;
}
promiseInfo.warned = true;
const { reason, uid } = promiseInfo;
if (state === states.strict) {
fatalException(reason);
}
if (!process.emit('unhandledRejection', reason, promise) ||
// Always warn in case the user requested it.
state === states.warn) {
emitWarning(uid, reason);
}
maybeScheduledTicks = true;
}
return maybeScheduledTicks || pendingUnhandledRejections.length !== 0;
}

function getError(name, message) {
// Reset the stack to prevent any overhead.
const tmp = Error.stackTraceLimit;
Error.stackTraceLimit = 0;
// eslint-disable-next-line no-restricted-syntax
const err = new Error(message);
Error.stackTraceLimit = tmp;
Object.defineProperty(err, 'name', {
value: name,
enumerable: false,
writable: true,
configurable: true,
});
return err;
}

function fatalException(reason) {
let err;
if (reason instanceof Error) {
err = reason;
} else {
err = getError(
'UnhandledPromiseRejection',
'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 "${safeToString(reason)}".`
);
err.code = 'ERR_UNHANDLED_REJECTION';
}
triggerFatalException(err, true /* fromPromise */);
}

function listenForRejections() {
setPromiseRejectCallback(promiseRejectHandler);
}
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/process/task_queues.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,11 @@ function runMicrotask() {
try {
callback();
} catch (error) {
// TODO(devsnek) remove this if
// TODO(devsnek): Remove this if
// https://bugs.chromium.org/p/v8/issues/detail?id=8326
// is resolved such that V8 triggers the fatal exception
// handler for microtasks
triggerFatalException(error);
// handler for microtasks.
triggerFatalException(error, false /* fromPromise */);
} finally {
this.emitDestroy();
}
Expand Down
15 changes: 13 additions & 2 deletions src/node_errors.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
namespace node {

using errors::TryCatchScope;
using v8::Boolean;
using v8::Context;
using v8::Exception;
using v8::Function;
Expand Down Expand Up @@ -771,7 +772,8 @@ void DecorateErrorStack(Environment* env,

void FatalException(Isolate* isolate,
Local<Value> error,
Local<Message> message) {
Local<Message> message,
bool from_promise) {
CHECK(!error.IsEmpty());
HandleScope scope(isolate);

Expand All @@ -794,9 +796,12 @@ void FatalException(Isolate* isolate,
// Do not call FatalException when _fatalException handler throws
fatal_try_catch.SetVerbose(false);

Local<Value> argv[2] = { error,
Boolean::New(env->isolate(), from_promise) };

// This will return true if the JS layer handled it, false otherwise
MaybeLocal<Value> caught = fatal_exception_function.As<Function>()->Call(
env->context(), process_object, 1, &error);
env->context(), process_object, arraysize(argv), argv);

if (fatal_try_catch.HasTerminated()) return;

Expand All @@ -821,4 +826,10 @@ void FatalException(Isolate* isolate,
}
}

void FatalException(Isolate* isolate,
Local<Value> error,
Local<Message> message) {
FatalException(isolate, error, message, false /* from_promise */);
}

} // namespace node
Loading