-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Changes from all commits
6ed1985
1eef8cf
46b0c97
5218bb2
2dfe16a
637824f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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` | ||
<!-- 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`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Silent is nice although I'd ideally prefer it if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used Any other opinions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with either - although There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, this might be me being greedy but yes - I'd like to be able to exit immediately whenever there is no 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 |
||
`ERROR_ON_GC`, `ERROR` or `STRICT` while the default behavior without using the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that when enabling
Am I missing something? If not, is this the intended behavior? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the docs (unhandled rejections that are garbage collected are There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this looks more like our node.js style callback now :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
fs.writeSync( | ||
1, | ||
`Caught exception: ${err}\n` + | ||
`Exception originated from unhandled rejection: ${errorOrigin}` | ||
); | ||
}); | ||
|
||
setTimeout(() => { | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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