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 capture function for uncaught exceptions #17159

Closed
Closed
Show file tree
Hide file tree
Changes from 2 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
5 changes: 5 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,10 @@ added: v0.10
Aborting instead of exiting causes a core file to be generated for post-mortem
analysis using a debugger (such as `lldb`, `gdb`, and `mdb`).

*Note*: If this flag is passed, the behavior can still be set to not abort
through [`process.setUncaughtExceptionCaptureCallback()`][] (and through usage
of the `domain` module that uses it).

### `--trace-warnings`
<!-- YAML
added: v6.0.0
Expand Down Expand Up @@ -598,3 +602,4 @@ greater than `4` (its current default value). For more information, see the
[debugger]: debugger.html
[emit_warning]: process.html#process_process_emitwarning_warning_type_code_ctor
[libuv threadpool documentation]: http://docs.libuv.org/en/latest/threadpool.html
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
18 changes: 18 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,23 @@ 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 couldn’t establish the required
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit: s/couldn't/could not

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, done!

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 @@ -1565,6 +1582,7 @@ Creation of a [`zlib`][] object failed due to incorrect configuration.
[`new URLSearchParams(iterable)`]: url.html#url_constructor_new_urlsearchparams_iterable
[`process.on('uncaughtException')`]: process.html#process_event_uncaughtexception
[`process.send()`]: process.html#process_process_send_message_sendhandle_options_callback
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[`require('crypto').setEngine()`]: crypto.html#crypto_crypto_setengine_engine_flags
[`server.listen()`]: net.html#net_server_listen
[ES6 module]: esm.html
Expand Down
37 changes: 37 additions & 0 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -1136,6 +1136,16 @@ if (process.getuid) {
*Note*: This function is only available on POSIX platforms (i.e. not Windows
or Android).

## process.hasUncaughtExceptionCaptureCallback()
<!-- YAML
added: REPLACEME
-->

* Returns: {boolean}

Indicates whether a callback has been set using
[`process.setUncaughtExceptionCaptureCallback()`][].

## process.hrtime([time])
<!-- YAML
added: v0.7.6
Expand Down Expand Up @@ -1637,6 +1647,29 @@ if (process.getuid && process.setuid) {
or Android).


## process.setUncaughtExceptionCaptureCallback(fn)
<!-- YAML
added: REPLACEME
-->

* `fn` {Function|null}

The `process.setUncaughtExceptionCapture` function sets a function that will
be invoked when an uncaught exception occurs, which will receive the exception
value itself as its first argument.

If such a function is set, the [`process.on('uncaughtException')`][] event will
not be emitted. If `--abort-on-uncaught-exception` was passed from the
command line or set through [`v8.setFlagsFromString()`][], the process will
not abort.

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

*Note*: Using this function is mutually exclusive with using the
deprecated [`domain`][] built-in module.

## process.stderr

* {Stream}
Expand Down Expand Up @@ -1921,6 +1954,7 @@ cases:
[`JSON.stringify` spec]: https://tc39.github.io/ecma262/#sec-json.stringify
[`console.error()`]: console.html#console_console_error_data_args
[`console.log()`]: console.html#console_console_log_data_args
[`domain`]: domain.html
[`end()`]: stream.html#stream_writable_end_chunk_encoding_callback
[`net.Server`]: net.html#net_class_net_server
[`net.Socket`]: net.html#net_class_net_socket
Expand All @@ -1930,11 +1964,14 @@ cases:
[`process.exit()`]: #process_process_exit_code
[`process.exitCode`]: #process_process_exitcode
[`process.kill()`]: #process_process_kill_pid_signal
[`process.on('uncaughtException')`]: process.html#process_event_uncaughtexception
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: out of ABC order)

Copy link
Member Author

Choose a reason for hiding this comment

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

Mh - it's o after c, right?

Copy link
Member

Choose a reason for hiding this comment

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

The one before it is promise not process. Tripped me up the first few times I read it.

Copy link
Contributor

Choose a reason for hiding this comment

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

process.kill
process.on
promise.catch ?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yup. They look so similar …

[`promise.catch()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/catch
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_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.

Out of order ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

… ;)

[`require()`]: globals.html#globals_require
[`require.main`]: modules.html#modules_accessing_the_main_module
[`require.resolve()`]: modules.html#modules_require_resolve_request_options
[`setTimeout(fn, 0)`]: timers.html#timers_settimeout_callback_delay_args
[`v8.setFlagsFromString()`]: v8.html#v8_v8_setflagsfromstring_flags
[Child Process]: child_process.html
[Cluster]: cluster.html
[Duplex]: stream.html#stream_duplex_and_transform_streams
Expand Down
84 changes: 73 additions & 11 deletions lib/domain.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

const util = require('util');
const EventEmitter = require('events');
const errors = require('internal/errors');
const { createHook } = require('async_hooks');

// communicate with events module, but don't require that
Expand Down Expand Up @@ -81,19 +82,77 @@ const asyncHook = createHook({
}
});

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

// Get the stack trace at the point where `domain` was required.
const domainRequireStack = new Error('require(`domain`) at this point').stack;

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

Choose a reason for hiding this comment

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

Nit: template string here?

Copy link
Member Author

Choose a reason for hiding this comment

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

But does that really make it more readable?

throw err;
};

// It's possible to enter one domain while already inside
// another one. The stack is each entered domain.
const stack = [];
exports._stack = stack;
process._setupDomainUse(stack);
process._setupDomainUse();

class Domain extends EventEmitter {
function updateExceptionCapture() {
if (stack.every((domain) => domain.listenerCount('error') === 0)) {
setUncaughtExceptionCaptureCallback(null);
} else {
setUncaughtExceptionCaptureCallback(null);
setUncaughtExceptionCaptureCallback((er) => {
return process.domain._errorHandler(er);
});
}
}


process.on('newListener', (name, listener) => {
if (name === 'uncaughtException' &&
listener !== domainUncaughtExceptionClear) {
// Make sure the first listener for `uncaughtException` always clears
// the domain stack.
process.removeListener(name, domainUncaughtExceptionClear);
process.prependListener(name, domainUncaughtExceptionClear);
}
});

process.on('removeListener', (name, listener) => {
if (name === 'uncaughtException' &&
listener !== domainUncaughtExceptionClear) {
// If the domain listener would be the only remaining one, remove it.
const listeners = process.listeners('uncaughtException');
if (listeners.length === 1 && listeners[0] === domainUncaughtExceptionClear)
process.removeListener(name, domainUncaughtExceptionClear);
}
});

function domainUncaughtExceptionClear() {
stack.length = 0;
exports.active = process.domain = null;
updateExceptionCapture();
}


class Domain extends EventEmitter {
constructor() {
super();

this.members = [];
asyncHook.enable();

this.on('removeListener', updateExceptionCapture);
this.on('newListener', updateExceptionCapture);
}
}

Expand Down Expand Up @@ -131,14 +190,14 @@ Domain.prototype._errorHandler = function _errorHandler(er) {
// prevent the process 'uncaughtException' event from being emitted
// if a listener is set.
if (EventEmitter.listenerCount(this, 'error') > 0) {
// 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);
try {
// Set the _emittingTopLevelDomainError 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
process._emittingTopLevelDomainError = true;
caught = this.emit('error', er);
} finally {
process._emittingTopLevelDomainError = false;
updateExceptionCapture();
}
}
} else {
Expand All @@ -161,20 +220,21 @@ Domain.prototype._errorHandler = function _errorHandler(er) {
if (this === exports.active) {
stack.pop();
}
updateExceptionCapture();
if (stack.length) {
exports.active = process.domain = stack[stack.length - 1];
caught = process._fatalException(er2);
caught = process.domain._errorHandler(er2);
} else {
caught = false;
// Pass on to the next exception handler.
throw er2;
}
}
}

// Exit all domains on the stack. Uncaught exceptions end the
// current tick and no domains should be left on the stack
// between ticks.
stack.length = 0;
exports.active = process.domain = null;
domainUncaughtExceptionClear();

return caught;
};
Expand All @@ -185,6 +245,7 @@ Domain.prototype.enter = function() {
// to push it onto the stack so that we can pop it later.
exports.active = process.domain = this;
stack.push(this);
updateExceptionCapture();
};


Expand All @@ -198,6 +259,7 @@ Domain.prototype.exit = function() {

exports.active = stack[stack.length - 1];
process.domain = exports.active;
updateExceptionCapture();
};


Expand Down
8 changes: 6 additions & 2 deletions lib/internal/bootstrap_node.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

(function(process) {
let internalBinding;
const exceptionHandlerState = { captureFn: null };

function startup() {
const EventEmitter = NativeModule.require('events');
Expand All @@ -35,6 +36,7 @@
const _process = NativeModule.require('internal/process');
_process.setupConfig(NativeModule._source);
_process.setupSignalHandlers();
_process.setupUncaughtExceptionCapture(exceptionHandlerState);
NativeModule.require('internal/process/warning').setup();
NativeModule.require('internal/process/next_tick').setup();
NativeModule.require('internal/process/stdio').setup();
Expand Down Expand Up @@ -377,8 +379,10 @@
// that threw and was never cleared. So clear it now.
async_id_fields[kInitTriggerAsyncId] = 0;

if (process.domain && process.domain._errorHandler)
caught = process.domain._errorHandler(er);
if (exceptionHandlerState.captureFn !== null) {
exceptionHandlerState.captureFn(er);
caught = true;
Copy link
Member

@apapirovski apapirovski Nov 27, 2017

Choose a reason for hiding this comment

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

I think this conflicts with ClearFatalExceptionHandler as it unsets process.domain but the captureFn would still exist and would throw again since process.domain._errorHandler would be undefined. That won't be an issue once my PR for events lands but in the meantime, I think we need to update this code:

node/src/node.cc

Lines 2479 to 2482 in bb44626

process->Set(
env->context(),
env->domain_string(),
Undefined(env->isolate())).FromJust();

We probably need better tests for that behaviour (I noticed it wasn't really tested while working on it myself)... unless I'm missing something with the above.

Copy link
Member Author

Choose a reason for hiding this comment

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

@apapirovski Since you brought it up in the other PR: I’m writing a PR that removes ClearFatalExceptionHandler altogether :)

Copy link
Member

Choose a reason for hiding this comment

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

@addaleax Sounds good! I guess it would either have to land before this or this would still need to be updated, right?

Out of curiosity, what direction are you going with it? (Because technically mine removed it too but I'm guessing you have a different solution in mind.)

Copy link
Member Author

Choose a reason for hiding this comment

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

@apapirovski Here’s the PR: #17333 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, and regarding updating this PR: The situation you’re worried about is a domain setting a capture function, that would proceed to capture the exception generated by one of these?

I think you are right, #17333 should probably land first (or at least this should not land without it)…

}

if (!caught)
caught = process.emit('uncaughtException', er);
Expand Down
7 changes: 7 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,13 @@ E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign');
E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH',
'Input buffers must have the same length');
E('ERR_DNS_SET_SERVERS_FAILED', 'c-ares failed to set servers: "%s" [%s]');
E('ERR_DOMAIN_CALLBACK_NOT_AVAILABLE',
'A callback was registered through ' +
'process.setUncaughtExceptionCaptureCallback(), which is mutually ' +
'exclusive with using the `domain` module');
E('ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE',
'The `domain` module is in use, which is mutually exclusive with calling ' +
'process.setUncaughtExceptionCaptureCallback()');
E('ERR_ENCODING_INVALID_ENCODED_DATA',
'The encoded data was not valid for encoding %s');
E('ERR_ENCODING_NOT_SUPPORTED', 'The "%s" encoding is not supported');
Expand Down
30 changes: 29 additions & 1 deletion lib/internal/process.js
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,33 @@ function setupRawDebug() {
};
}


function setupUncaughtExceptionCapture(exceptionHandlerState) {
// This is a typed array for faster communication with JS.
const shouldAbortOnUncaughtToggle = process._shouldAbortOnUncaughtToggle;
delete process._shouldAbortOnUncaughtToggle;

process.setUncaughtExceptionCaptureCallback = function(fn) {
if (fn === null) {
exceptionHandlerState.captureFn = fn;
shouldAbortOnUncaughtToggle[0] = 1;
return;
}
if (exceptionHandlerState.captureFn !== null) {
throw new errors.Error('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.

This error doesn't appear to be documented.

}
if (typeof fn !== 'function') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fn', 'Function');
Copy link
Member

@AndreasMadsen AndreasMadsen Nov 27, 2017

Choose a reason for hiding this comment

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

I would prefer type validation before the state validation.

}
exceptionHandlerState.captureFn = fn;
shouldAbortOnUncaughtToggle[0] = 0;
};

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

module.exports = {
setup_performance,
setup_cpuUsage,
Expand All @@ -256,5 +283,6 @@ module.exports = {
setupKillAndExit,
setupSignalHandlers,
setupChannel,
setupRawDebug
setupRawDebug,
setupUncaughtExceptionCapture
};
9 changes: 9 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ inline Environment::Environment(IsolateData* isolate_data,
emit_napi_warning_(true),
makecallback_cntr_(0),
scheduled_immediate_count_(isolate_, 1),
should_abort_on_uncaught_toggle_(isolate_, 1),
#if HAVE_INSPECTOR
inspector_agent_(new inspector::Agent(this)),
#endif
Expand Down Expand Up @@ -305,6 +306,9 @@ inline Environment::Environment(IsolateData* isolate_data,
performance_state_->milestones[
performance::NODE_PERFORMANCE_MILESTONE_V8_START] =
performance::performance_v8_start;

// By default, always abort when --abort-on-uncaught-exception was passed.
should_abort_on_uncaught_toggle_[0] = 1;
}

inline Environment::~Environment() {
Expand Down Expand Up @@ -399,6 +403,11 @@ inline void Environment::set_abort_on_uncaught_exception(bool value) {
abort_on_uncaught_exception_ = value;
}

inline AliasedBuffer<uint32_t, v8::Uint32Array>&
Environment::should_abort_on_uncaught_toggle() {
return should_abort_on_uncaught_toggle_;
}

inline std::vector<double>* Environment::destroy_async_id_list() {
return &destroy_async_id_list_;
}
Expand Down
Loading