Skip to content

Commit

Permalink
process: allow multiple uncaught exception capture calbacks
Browse files Browse the repository at this point in the history
  • Loading branch information
Julien Gilli committed Nov 9, 2018
1 parent 8884a98 commit 7082a27
Show file tree
Hide file tree
Showing 16 changed files with 57 additions and 139 deletions.
2 changes: 1 addition & 1 deletion doc/api/cli.md
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
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

[`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
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)
<!-- YAML
added: v9.3.0
-->

* `owner` {symbol}
* `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
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
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');

// 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);
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
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()) {
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ function setupChildProcessIpcChannel() {
}
}

process.domainsMap = new Map();

module.exports = {
setupStdio,
setupProcessMethods,
Expand Down
16 changes: 9 additions & 7 deletions lib/internal/process/per_thread.js
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);
}
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
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
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.

This file was deleted.

12 changes: 5 additions & 7 deletions test/parallel/test-process-exception-capture-errors.js
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

0 comments on commit 7082a27

Please sign in to comment.