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

src: exit on gc unhandled rejection #20097

Closed
wants to merge 6 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
3 changes: 2 additions & 1 deletion configure
Original file line number Diff line number Diff line change
Expand Up @@ -1140,7 +1140,8 @@ def configure_v8(o):
o['variables']['v8_no_strict_aliasing'] = 1 # Work around compiler bugs.
o['variables']['v8_optimized_debug'] = 0 # Compile with -O0 in debug builds.
o['variables']['v8_random_seed'] = 0 # Use a random seed for hash tables.
o['variables']['v8_promise_internal_field_count'] = 1 # Add internal field to promises for async hooks.
# Add internal field to promises for async hooks and unhandled rejections.
o['variables']['v8_promise_internal_field_count'] = 2
o['variables']['v8_use_snapshot'] = 'false' if options.without_snapshot else 'true'
o['variables']['v8_trace_maps'] = 1 if options.trace_maps else 0
o['variables']['node_use_v8_platform'] = b(not options.without_v8_platform)
Expand Down
2 changes: 1 addition & 1 deletion deps/v8/gypfiles/features.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@
'defines': ['ENABLE_DISASSEMBLER',],
}],
['v8_promise_internal_field_count!=0', {
'defines': ['V8_PROMISE_INTERNAL_FIELD_COUNT','v8_promise_internal_field_count'],
'defines': ['V8_PROMISE_INTERNAL_FIELD_COUNT=<(v8_promise_internal_field_count)'],
}],
['v8_enable_gdbjit==1', {
'defines': ['ENABLE_GDB_JIT_INTERFACE',],
Expand Down
60 changes: 60 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,33 @@ Write process warnings to the given file instead of printing to stderr. The
file will be created if it does not exist, and will be appended to if it does.
If an error occurs while attempting to write the warning to the file, the
warning will be written to stderr instead.

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

See below ..





















### `--throw-deprecation`
<!-- YAML
Expand Down Expand Up @@ -625,6 +652,36 @@ Path to the file used to store the persistent REPL history. The default path is
`~/.node_repl_history`, which is overridden by this variable. Setting the value
to an empty string (`''` or `' '`) disables persistent REPL history.

### `NODE_UNHANDLED_REJECTION=state`
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention here the default behaviour?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not do that anywhere else. However, it is described in the first paragraph, so I believe that should suffice.

Copy link
Member

Choose a reason for hiding this comment

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

Let's not add a new environment variable. Let's just use the environment variable and allow it to be set in NODE_OPTIONS

<!-- YAML
added: REPLACEME
-->

Setting this environment variable allows fine grained control over the behavior
BridgeAR marked this conversation as resolved.
Show resolved Hide resolved
of unhandled rejections. This may be set to either one of `SILENT`, `WARN`,
Copy link
Member

Choose a reason for hiding this comment

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

Silent is nice although NONE might be more appropriate?

I'd ideally prefer it if ERROR was named strict but I'm also fine with 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 used SILENT and ERROR because they represent what happens. NONE is probably also fine but with STRICT it is not clear what it stands for. At least that's how it feels like to me.

Any other opinions?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with either - although ERROR doesn't do what I'd expect it to do (I'd like it to exit immediately on any rejection getting to SetPromiseRejectCallback when not synchronously handled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want that to be absolutely sure all catch handlers are attached sync? We could add another state for this. I have no strong feeling about either. I just thought it will likely be best for users in real applications because some modules might add a handler later on.

Copy link
Member

Choose a reason for hiding this comment

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

Do you want that to be absolutely sure all catch handlers are attached sync?

Well, this might be me being greedy but yes - I'd like to be able to exit immediately whenever there is no catch handler attached synchronously (before other microticks have passed).

I think that a lot of us in core feel this way but are afraid to recommend it because of the semantics - if we have a none option which makes debugging really hard we might as well have a strict mode which makes debugging really easy :D

`ERROR_ON_GC`, `ERROR` or `STRICT` while the default behavior without using the
Copy link
Member

Choose a reason for hiding this comment

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

just a nit... make the values lower-case. They tend to be easier for users. --unhandled-rejections=error_on_gc

Copy link
Member Author

Choose a reason for hiding this comment

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

They will be accepted in all cases but it should be easier to read them being upper case.

environment variable is the same as `WARN`. `WARN` reflects the
[`unhandledRejection hook`][]. The [`unhandledRejection hook`][] functionality
is not influenced by any of these options.

Setting the environment variable to `SILENT` prevents unhandled rejection
warnings from being logged, even if no [`unhandledRejection hook`][] is
attached.

If set to `ERROR_ON_GC` unhandled rejections that are [garbage collected][] are
raised as uncaught exceptions.

Choose a reason for hiding this comment

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

It seems that when enabling --abort-on-uncaught-exception, these exceptions do not make the process abort:

$ ulimit -c
unlimited
$ ../../node --expose-gc --abort-on-uncaught-exception promise_unhandled_reject.js
First GC did not collect it.
Error: Two
    at Promise (/Users/jgilli/dev/node-ruben-pr/test/message/promise_unhandled_reject.js:18:9)
    at new Promise (<anonymous>)
    at Object.<anonymous> (/Users/jgilli/dev/node-ruben-pr/test/message/promise_unhandled_reject.js:17:10)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)
    at startup (internal/bootstrap/node.js:241:19) Promise {
  <rejected> Error: Two
    at Promise (/Users/jgilli/dev/node-ruben-pr/test/message/promise_unhandled_reject.js:18:9)
    at new Promise (<anonymous>)
    at Object.<anonymous> (/Users/jgilli/dev/node-ruben-pr/test/message/promise_unhandled_reject.js:17:10)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)
    at startup (internal/bootstrap/node.js:241:19) }
Error: Three
    at Object.<anonymous> (/Users/jgilli/dev/node-ruben-pr/test/message/promise_unhandled_reject.js:21:27)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)
    at startup (internal/bootstrap/node.js:241:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:577:3) Promise {
  <rejected> Error: Three
    at Object.<anonymous> (/Users/jgilli/dev/node-ruben-pr/test/message/promise_unhandled_reject.js:21:27)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)
    at startup (internal/bootstrap/node.js:241:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:577:3) }
Second GC did not collect it.
Exit code 1
undefined:0



Error: Two
    at Promise (/Users/jgilli/dev/node-ruben-pr/test/message/promise_unhandled_reject.js:18:9)
    at new Promise (<anonymous>)
    at Object.<anonymous> (/Users/jgilli/dev/node-ruben-pr/test/message/promise_unhandled_reject.js:17:10)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)
    at startup (internal/bootstrap/node.js:241:19)
$ echo $?
1
$ ls /cores/
$ 

Am I missing something? If not, is this the intended behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the docs (unhandled rejections that are garbage collected are
raised as uncaught exceptions) it seems like the behavior is incorrect and ERROR_ON_GC should generate a core dump when used with --abort-on-uncaught-exception

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 agree and I'll fix this. @misterdjules thanks for bringing this up by the way 👍 I did not think about this case before.


If set to `ERROR` all rejections that are not handled after a single
`nextTick()` has passed are raised as uncaught exception.

If set to `STRICT` all rejections that are not handled synchronous are raised as
uncaught exception.

Any rejection that is raised as exception can be caught by the
[`uncaughtException hook`][]. Handling promises can be done lazily by adding a
catch handler to the promise chain after the promise was already potentially
rejected.

### `OPENSSL_CONF=file`
<!-- YAML
added: v6.11.0
Expand Down Expand Up @@ -689,9 +746,12 @@ greater than `4` (its current default value). For more information, see the
[`Buffer`]: buffer.html#buffer_class_buffer
[`SlowBuffer`]: buffer.html#buffer_class_slowbuffer
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[`uncaughtException hook`]: process.html#process_event_uncaughtexception
[`unhandledRejection hook`]: process.html#process_event_unhandledrejection
[Chrome DevTools Protocol]: https://chromedevtools.github.io/devtools-protocol/
[REPL]: repl.html
[debugger]: debugger.html
[emit_warning]: process.html#process_process_emitwarning_warning_type_code_ctor
[experimental ECMAScript Module]: esm.html#esm_loader_hooks
[garbage collected]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Memory_Management#Garbage_collection
[libuv threadpool documentation]: http://docs.libuv.org/en/latest/threadpool.html
6 changes: 3 additions & 3 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -1057,16 +1057,16 @@ future release.
[`Buffer.isBuffer()`]: buffer.html#buffer_class_method_buffer_isbuffer_obj
[`Cipher`]: crypto.html#crypto_class_cipher
[`Decipher`]: crypto.html#crypto_class_decipher
[`assert`]: assert.html
[`clearInterval()`]: timers.html#timers_clearinterval_timeout
[`clearTimeout()`]: timers.html#timers_cleartimeout_timeout
[`EventEmitter.listenerCount(emitter, eventName)`]: events.html#events_eventemitter_listenercount_emitter_eventname
[`Server.connections`]: net.html#net_server_connections
[`Server.getConnections()`]: net.html#net_server_getconnections_callback
[`Server.listen({fd: <number>})`]: net.html#net_server_listen_handle_backlog_callback
[`SlowBuffer`]: buffer.html#buffer_class_slowbuffer
[`assert`]: assert.html
[`asyncResource.runInAsyncScope()`]: async_hooks.html#async_hooks_asyncresource_runinasyncscope_fn_thisarg_args
[`child_process`]: child_process.html
[`clearInterval()`]: timers.html#timers_clearinterval_timeout
[`clearTimeout()`]: timers.html#timers_cleartimeout_timeout
[`console.error()`]: console.html#console_console_error_data_args
[`console.log()`]: console.html#console_console_log_data_args
[`crypto.createCipher()`]: crypto.html#crypto_crypto_createcipher_algorithm_password_options
Expand Down
6 changes: 6 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1158,6 +1158,12 @@ Invalid characters were detected in headers.
A cursor on a given stream cannot be moved to a specified row without a
specified column.

<a id="ERR_INVALID_ENV_VALUE"></a>
### ERR_INVALID_ENV_VALUE

An environment variable value which is unrecognized. Please consult the
documentation and make sure to pass in valid values.

<a id="ERR_INVALID_FD"></a>
### ERR_INVALID_FD

Expand Down
45 changes: 26 additions & 19 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,17 @@ most convenient for scripts).
### Event: 'uncaughtException'
<!-- YAML
added: v0.1.18
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/20097
description: Added the `errorOrigin` argument.
-->

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

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 @@ -159,12 +168,13 @@ behavior. You may also change the [`process.exitCode`][] in
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, errorOrigin) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this looks more like our node.js style callback now :D

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 unhandledRejection hook looks similar as well. But I guess you mean this because of the err as first argument? I would like to have a different way dealing with this but after playing with a alternative, I decided to still stick to this.

fs.writeSync(
1,
`Caught exception: ${err}\n` +
`Exception originated from unhandled rejection: ${errorOrigin}`
);
});

setTimeout(() => {
Expand Down Expand Up @@ -216,6 +226,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 @@ -224,21 +238,15 @@ 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);
// application specific logging, throwing an error, or other logic here
process.on('unhandledRejection', (reason, promise) => {
console.log('Unhandled Rejection at:', promise, 'reason:', reason);
// Application specific logging, throwing an error, or other logic here
});

somePromise.then((res) => {
return reportToUser(JSON.pasre(res)); // note the typo (`pasre`)
}); // no `.catch()` or `.then()`
return reportToUser(JSON.pasre(res)); // Note the typo (`pasre`)
}); // No `.catch()` or `.then()`
```

The following will also trigger the `'unhandledRejection'` event to be
Expand All @@ -251,15 +259,15 @@ function SomeResource() {
}

const resource = new SomeResource();
// no .catch or .then on resource.loaded for at least a turn
// No .catch or .then on resource.loaded for at least a turn
```

In this example case, it is possible to track the rejection as a developer error
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 @@ -2089,7 +2097,6 @@ cases:
[`'exit'`]: #process_event_exit
[`'finish'`]: stream.html#stream_event_finish
[`'message'`]: child_process.html#child_process_event_message
[`'rejectionHandled'`]: #process_event_rejectionhandled
BridgeAR marked this conversation as resolved.
Show resolved Hide resolved
[`'uncaughtException'`]: #process_event_uncaughtexception
[`ChildProcess.disconnect()`]: child_process.html#child_process_subprocess_disconnect
[`subprocess.kill()`]: child_process.html#child_process_subprocess_kill_signal
Expand Down
15 changes: 12 additions & 3 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -518,14 +518,23 @@
emitAfter
} = NativeModule.require('internal/async_hooks');

process._fatalException = function(er) {
const {
unhandledState,
states
} = NativeModule.require('internal/process/promises');

process._fatalException = function(er, fromPromise = false) {
if (fromPromise && unhandledState() <= states.WARN) {
return true;
}
// It's possible that defaultTriggerAsyncId was set for a constructor
// call that threw and was never cleared. So clear it now.
clearDefaultTriggerAsyncId();

const from = fromPromise ? 'FROM_PROMISE' : 'FROM_ERROR';
if (exceptionHandlerState.captureFn !== null) {
exceptionHandlerState.captureFn(er);
} else if (!process.emit('uncaughtException', er)) {
} else if (!process.emit('uncaughtException', er, from)) {
// 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 All @@ -541,7 +550,7 @@
const { kExpandStackSymbol } = NativeModule.require('internal/util');
if (typeof er[kExpandStackSymbol] === 'function')
er[kExpandStackSymbol]();
} catch (er) {}
} catch {}
return false;
}

Expand Down
2 changes: 2 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,8 @@ E('ERR_INVALID_CHAR',
}, TypeError);
E('ERR_INVALID_CURSOR_POS',
'Cannot set cursor row without setting its column', TypeError);
E('ERR_INVALID_ENV_VALUE',
'The value "%s" for environment variable %s is invalid.', TypeError);
E('ERR_INVALID_FD',
'"fd" must be a positive integer: %s', RangeError);
E('ERR_INVALID_FD_TYPE', 'Unsupported fd type: %s', TypeError);
Expand Down
Loading