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

process: allow multiple uncaught exception capture calbacks #24279

Closed
wants to merge 1 commit into from
Closed

process: allow multiple uncaught exception capture calbacks #24279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,7 @@ greater than `4` (its current default value). For more information, see the
[`--openssl-config`]: #cli_openssl_config_file
[`Buffer`]: buffer.html#buffer_class_buffer
[`SlowBuffer`]: buffer.html#buffer_class_slowbuffer
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_owner_fn
[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
27 changes: 0 additions & 27 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,23 +803,6 @@ A signing `key` was not provided to the [`sign.sign()`][] method.

`c-ares` failed to set the DNS server.

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

The `domain` module was not usable since it could not establish the required
error handling hooks, because
[`process.setUncaughtExceptionCaptureCallback()`][] had been called at an
earlier point in time.

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

[`process.setUncaughtExceptionCaptureCallback()`][] could not be called
because the `domain` module has been loaded at an earlier point in time.

The stack trace is extended to include the point in time at which the
`domain` module had been loaded.

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

Expand Down Expand Up @@ -1720,15 +1703,6 @@ A `Transform` stream finished with data still in the write buffer.

The initialization of a TTY failed due to a system error.

<a id="ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET"></a>
### ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET
Copy link
Member

Choose a reason for hiding this comment

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

We have a section for errors that were once present but are no longer below in this document :)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the pointer 👍 I'll add those errors to the Legacy Node.js Error Codes section. Just to make sure: is it the section you were referring to?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly. :)


[`process.setUncaughtExceptionCaptureCallback()`][] was called twice,
without first resetting the callback to `null`.

This error is designed to prevent accidentally overwriting a callback registered
from another module.

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

Expand Down Expand Up @@ -2153,7 +2127,6 @@ such as `process.stdout.on('data')`.
[`new URL(input)`]: url.html#url_constructor_new_url_input_base
[`new URLSearchParams(iterable)`]: url.html#url_constructor_new_urlsearchparams_iterable
[`process.send()`]: process.html#process_process_send_message_sendhandle_options_callback
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[`readable._read()`]: stream.html#stream_readable_read_size_1
[`require('crypto').setEngine()`]: crypto.html#crypto_crypto_setengine_engine_flags
[`require()`]: modules.html#modules_require
Expand Down
13 changes: 4 additions & 9 deletions doc/api/process.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1807,11 +1807,12 @@ This function is only available on POSIX platforms (i.e. not Windows or
Android).
This feature is not available in [`Worker`][] threads.

## process.setUncaughtExceptionCaptureCallback(fn)
## process.setUncaughtExceptionCaptureCallback(owner, fn)
Copy link
Member

Choose a reason for hiding this comment

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

A bit concerned about backwards compat here. Is the new owner argument optional? What about existing code that may be calling setUncaughtExceptionCaptureCallback(fn)?

Copy link
Member

Choose a reason for hiding this comment

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

why not use a Set instead of a Map and use the functions themselves as keys

Copy link
Author

Choose a reason for hiding this comment

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

@devsnek's suggestion seems like a good alternative, I'll try that 👍

<!-- YAML
added: v9.3.0
-->

* `owner` {symbol}
Copy link
Author

Choose a reason for hiding this comment

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

owner is a symbol so that each domain-like implementation can use a unique key to register their own capture callbacks.

* `fn` {Function|null}

The `process.setUncaughtExceptionCaptureCallback()` function sets a function
Expand All @@ -1824,12 +1825,7 @@ command line or set through [`v8.setFlagsFromString()`][], the process will
not abort.

To unset the capture function,
`process.setUncaughtExceptionCaptureCallback(null)` may be used. Calling this
method with a non-`null` argument while another capture function is set will
throw an error.

Using this function is mutually exclusive with using the deprecated
[`domain`][] built-in module.
`process.setUncaughtExceptionCaptureCallback(owner, null)` may be used.

## process.stderr

Expand Down Expand Up @@ -2137,7 +2133,6 @@ cases:
[`Worker`]: worker_threads.html#worker_threads_class_worker
[`console.error()`]: console.html#console_console_error_data_args
[`console.log()`]: console.html#console_console_log_data_args
[`domain`]: domain.html
[`net.Server`]: net.html#net_class_net_server
[`net.Socket`]: net.html#net_class_net_socket
[`NODE_OPTIONS`]: cli.html#cli_node_options_options
Expand All @@ -2150,7 +2145,7 @@ cases:
[`process.hrtime()`]: #process_process_hrtime_time
[`process.hrtime.bigint()`]: #process_process_hrtime_bigint
[`process.kill()`]: #process_process_kill_pid_signal
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_owner_fn
[`promise.catch()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/catch
[`require()`]: globals.html#globals_require
[`require.main`]: modules.html#modules_accessing_the_main_module
Expand Down
9 changes: 2 additions & 7 deletions doc/api/repl.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,8 @@ global or scoped variable, the input `fs` will be evaluated on-demand as
The REPL uses the [`domain`][] module to catch all uncaught exceptions for that
REPL session.

This use of the [`domain`][] module in the REPL has these side effects:

* Uncaught exceptions do not emit the [`'uncaughtException'`][] event.
* Trying to use [`process.setUncaughtExceptionCaptureCallback()`][] throws
an [`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`][] error.
This use of the [`domain`][] module in the REPL has the side effects of making
uncaught exceptions not emit the [`'uncaughtException'`][] event.

#### Assignment of the `_` (underscore) variable
<!-- YAML
Expand Down Expand Up @@ -627,9 +624,7 @@ For an example of running a REPL instance over [curl(1)][], see:

[`'uncaughtException'`]: process.html#process_event_uncaughtexception
[`--experimental-repl-await`]: cli.html#cli_experimental_repl_await
[`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`]: errors.html#errors_err_domain_cannot_set_uncaught_exception_capture
[`domain`]: domain.html
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[`readline.InterfaceCompleter`]: readline.html#readline_use_of_the_completer_function
[`readline.Interface`]: readline.html#readline_class_interface
[`repl.ReplServer`]: #repl_class_replserver
Expand Down
28 changes: 6 additions & 22 deletions lib/domain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@
const util = require('util');
const EventEmitter = require('events');
const {
ERR_DOMAIN_CALLBACK_NOT_AVAILABLE,
ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE,
ERR_UNHANDLED_ERROR
} = require('internal/errors').codes;
const { createHook } = require('async_hooks');

const CAPTURE_CB_KEY = Symbol('domain');
Copy link
Member

Choose a reason for hiding this comment

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

Can we do something similar to what https://github.com/nodejs/node/blob/master/doc/guides/using-symbols.md suggests? e.g.

Suggested change
const CAPTURE_CB_KEY = Symbol('domain');
const kCaptureKeyDomain = Symbol('kCaptureKeyDomain');

Having identical names for the variable and the symbol itself helps with grepping :)

Copy link
Author

Choose a reason for hiding this comment

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

Great suggestion, I'll do that 👍


// overwrite process.domain with a getter/setter that will allow for more
// effective optimizations
var _domain = [null];
Expand Down Expand Up @@ -74,23 +74,7 @@ const asyncHook = createHook({
}
});

// When domains are in use, they claim full ownership of the
// uncaught exception capture callback.
if (process.hasUncaughtExceptionCaptureCallback()) {
throw new ERR_DOMAIN_CALLBACK_NOT_AVAILABLE();
}

// Get the stack trace at the point where `domain` was required.
// eslint-disable-next-line no-restricted-syntax
const domainRequireStack = new Error('require(`domain`) at this point').stack;

const { setUncaughtExceptionCaptureCallback } = process;
process.setUncaughtExceptionCaptureCallback = function(fn) {
const err = new ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE();
err.stack = err.stack + '\n' + '-'.repeat(40) + '\n' + domainRequireStack;
throw err;
};


let sendMakeCallbackDeprecation = false;
function emitMakeCallbackDeprecation() {
Expand Down Expand Up @@ -125,10 +109,10 @@ internalBinding('domain').enable(topLevelDomainCallback);

function updateExceptionCapture() {
if (stack.every((domain) => domain.listenerCount('error') === 0)) {
setUncaughtExceptionCaptureCallback(null);
setUncaughtExceptionCaptureCallback(CAPTURE_CB_KEY, null);
} else {
setUncaughtExceptionCaptureCallback(null);
setUncaughtExceptionCaptureCallback((er) => {
setUncaughtExceptionCaptureCallback(CAPTURE_CB_KEY, null);
Copy link
Author

Choose a reason for hiding this comment

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

Setting this to null before setting it to an actual callback is actually not needed anymore, I'll update this shortly.

setUncaughtExceptionCaptureCallback(CAPTURE_CB_KEY, (er) => {
return process.domain._errorHandler(er);
});
}
Expand Down Expand Up @@ -211,7 +195,7 @@ Domain.prototype._errorHandler = function(er) {
// Clear the uncaughtExceptionCaptureCallback so that we know that, even
// if technically the top-level domain is still active, it would
// be ok to abort on an uncaught exception at this point
setUncaughtExceptionCaptureCallback(null);
setUncaughtExceptionCaptureCallback(CAPTURE_CB_KEY, null);
try {
caught = this.emit('error', er);
} finally {
Expand Down
8 changes: 5 additions & 3 deletions lib/internal/bootstrap/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
_shouldAbortOnUncaughtToggle },
{ internalBinding, NativeModule },
triggerFatalException) {
const exceptionHandlerState = { captureFn: null };
const exceptionHandlerState = { captureFns: new Map() };
const isMainThread = internalBinding('worker').threadId === 0;

function startup() {
Expand Down Expand Up @@ -579,8 +579,10 @@
// call that threw and was never cleared. So clear it now.
clearDefaultTriggerAsyncId();

if (exceptionHandlerState.captureFn !== null) {
exceptionHandlerState.captureFn(er);
if (exceptionHandlerState.captureFns.size !== 0) {
for (const fn of exceptionHandlerState.captureFns.values()) {
Copy link
Member

@devsnek devsnek Nov 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

the branch into multiple ownership here feels... off. these kinds of handlers should be at an application level, so the possibility of having like 30 functions all handling an uncaught exception seems a bit confusing

Copy link
Author

Choose a reason for hiding this comment

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

@devsnek Did you read the motivation for these changes in #23348? If so, what alternative would you suggest to achieve the goals presented there?

Copy link
Member

@devsnek devsnek Nov 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@misterdjules if we had some sort of stopPropagation behaviour, i think this would be doable.

maybe

for (const fn of exceptionHandlerState.captureFns.values()) {
  if (fn() !== false) { break; } // return `false` to signal you didn't handle it, and the next function should be used
}

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @devsnek – the purpose of these hooks is to (possibly?) take ownership for the errors, otherwise we’d basically end up with a list of uncaughtException listeners again

Copy link
Author

Choose a reason for hiding this comment

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

The purpose of calling all (possibly many) registered callbacks was to address the use case tested by https://github.com/misterdjules/domaine/blob/master/test/test-domaine-domain-compat.js.

Basically, if one module runs a function within a user-land implemented domain-like module and that function runs another function within another domain-like instance, when an error is thrown in that second function, the goal would be to have both domain-like instances to handle that same error. The rationale for this is that we'd want those different domain-like implementations to not step on each other by handling errors for each other (think for instance of the case where those domain-like instances are created by different dependencies of the same Node app).

However, this is an opinionated design decision and I think I'd be totally fine with only allowing the top-level active domain-like instance (d2 in the example mentioned above) to handle uncaught errors.

In that case though it seems we'd need to be able to keep a single stack of all different domain-like instances and that exposing an API to set the capture callback to user-land is not useful, as there would be a need for setting one of them internally in core. @devsnek @addaleax What are your thoughts on this?

fn(er);
}
} else if (!process.emit('uncaughtException', er)) {
// If someone handled it, then great. otherwise, die in C++ land
// since that means that we'll exit the process, emit the 'exit' event.
Expand Down
13 changes: 0 additions & 13 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,15 +563,6 @@ E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH',
'Input buffers must have the same length', RangeError);
E('ERR_DNS_SET_SERVERS_FAILED', 'c-ares failed to set servers: "%s" [%s]',
Error);
E('ERR_DOMAIN_CALLBACK_NOT_AVAILABLE',
'A callback was registered through ' +
'process.setUncaughtExceptionCaptureCallback(), which is mutually ' +
'exclusive with using the `domain` module',
Error);
E('ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE',
'The `domain` module is in use, which is mutually exclusive with calling ' +
'process.setUncaughtExceptionCaptureCallback()',
Error);
E('ERR_ENCODING_INVALID_ENCODED_DATA',
'The encoded data was not valid for encoding %s', TypeError);
E('ERR_ENCODING_NOT_SUPPORTED', 'The "%s" encoding is not supported',
Expand Down Expand Up @@ -885,10 +876,6 @@ E('ERR_TRANSFORM_ALREADY_TRANSFORMING',
E('ERR_TRANSFORM_WITH_LENGTH_0',
'Calling transform done when writableState.length != 0', Error);
E('ERR_TTY_INIT_FAILED', 'TTY initialization failed', SystemError);
E('ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET',
'`process.setupUncaughtExceptionCapture()` was called while a capture ' +
'callback was already active',
Error);
E('ERR_UNESCAPED_CHARACTERS', '%s contains unescaped characters', TypeError);
E('ERR_UNHANDLED_ERROR',
// Using a default argument here is important so the argument is not counted
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/process/main_thread_only.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ function setupChildProcessIpcChannel() {
}
}

process.domainsMap = new Map();
Copy link
Author

Choose a reason for hiding this comment

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

process.domainsMap is only needed to provide a way for applications to retrieve active domains, in the same way the currently active domain instance is available as process.domain. This might not necessarily be required though, and is here in this PR for now just to accommodate the REPL use case, which depends on being able to access any active domain in order to forward exceptions that are raised and caught by it to active domain instances..


module.exports = {
setupStdio,
setupProcessMethods,
Expand Down
16 changes: 9 additions & 7 deletions lib/internal/process/per_thread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_OPT_VALUE,
ERR_OUT_OF_RANGE,
ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET,
ERR_UNKNOWN_SIGNAL
}
} = require('internal/errors');
Expand Down Expand Up @@ -213,24 +212,27 @@ function setupUncaughtExceptionCapture(exceptionHandlerState,
// shouldAbortOnUncaughtToggle is a typed array for faster
// communication with JS.

process.setUncaughtExceptionCaptureCallback = function(fn) {
process.setUncaughtExceptionCaptureCallback = function(owner, fn) {
if (fn === null) {
exceptionHandlerState.captureFn = fn;
exceptionHandlerState.captureFns.delete(owner);
shouldAbortOnUncaughtToggle[0] = 1;
return;
}

if (typeof fn !== 'function') {
throw new ERR_INVALID_ARG_TYPE('fn', ['Function', 'null'], fn);
}
if (exceptionHandlerState.captureFn !== null) {
throw new ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET();

if (typeof owner !== 'symbol') {
throw new ERR_INVALID_ARG_TYPE('owner', ['Symbol'], owner);
Copy link
Member

Choose a reason for hiding this comment

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

I guess this makes this semver-major?

Copy link
Member

Choose a reason for hiding this comment

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

why not use a Set and just add/remove the functions themselves

Copy link
Author

Choose a reason for hiding this comment

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

@devsnek's suggestion seems like a good alternative, I'll try that 👍

}
exceptionHandlerState.captureFn = fn;

exceptionHandlerState.captureFns.set(owner, fn);
shouldAbortOnUncaughtToggle[0] = 0;
};

process.hasUncaughtExceptionCaptureCallback = function() {
return exceptionHandlerState.captureFn !== null;
return exceptionHandlerState.captureFns.size !== 0;
};
}

Expand Down
12 changes: 12 additions & 0 deletions lib/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,12 +348,24 @@ function REPLServer(prompt,
}
}
} catch (e) {
let forwardedToDomain = false;
err = e;

if (process.domain) {
debug('not recoverable, send to domain');
forwardedToDomain = true;
process.domain.emit('error', err);
process.domain.exit();
}

debug('domainsMap values:' + process.domainsMap.values());
for (const d of process.domainsMap.values()) {
forwardedToDomain = true;
d.emit('error', err);
d.exit();
}

if (forwardedToDomain) {
return;
}
}
Expand Down
14 changes: 2 additions & 12 deletions test/parallel/test-domain-load-after-set-uncaught-exception-capture.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,6 @@
'use strict';
const common = require('../common');
const captureSym = Symbol('foo');

process.setUncaughtExceptionCaptureCallback(common.mustNotCall());

common.expectsError(
() => require('domain'),
{
code: 'ERR_DOMAIN_CALLBACK_NOT_AVAILABLE',
type: Error,
message: /^A callback was registered.*with using the `domain` module/
}
);

process.setUncaughtExceptionCaptureCallback(null);
process.setUncaughtExceptionCaptureCallback(captureSym, common.mustNotCall());
require('domain'); // Should not throw.
28 changes: 0 additions & 28 deletions test/parallel/test-domain-set-uncaught-exception-capture-after-load.js
View file
Open in desktop

This file was deleted.

12 changes: 5 additions & 7 deletions test/parallel/test-process-exception-capture-errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
const common = require('../common');

common.expectsError(
() => process.setUncaughtExceptionCaptureCallback(42),
() => process.setUncaughtExceptionCaptureCallback(Symbol('foo'), 42),
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
Expand All @@ -11,13 +11,11 @@ common.expectsError(
}
);

process.setUncaughtExceptionCaptureCallback(common.mustNotCall());

common.expectsError(
() => process.setUncaughtExceptionCaptureCallback(common.mustNotCall()),
() => process.setUncaughtExceptionCaptureCallback('foo', () => {}),
{
code: 'ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET',
type: Error,
message: /setupUncaughtExceptionCapture.*called while a capture callback/
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "owner" argument must be of type Symbol. Received type string'
}
);
Loading