Skip to content

Commit

Permalink
domains: fix handling of uncaught exceptions
Browse files Browse the repository at this point in the history
Fix node exiting due to an exception being thrown rather than emitting
an 'uncaughtException' event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an 'uncaughtException' event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and --abort-on-uncaught-exception is used.

Fixes #3607 and #3653.

PR: #3887
PR-URL: #3887
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
Julien Gilli authored and Myles Borins committed Feb 11, 2016
1 parent 53bae2d commit 563c359
Show file tree
Hide file tree
Showing 9 changed files with 878 additions and 58 deletions.
11 changes: 6 additions & 5 deletions lib/domain.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,20 @@ var endMethods = ['end', 'abort', 'destroy', 'destroySoon'];
// a few side effects.
events.usingDomains = true;

// it's possible to enter one domain while already inside
// another one. the stack is each entered domain.
var stack = [];
exports._stack = stack;

// let the process know we're using domains
process._usingDomains();
process._usingDomains(stack);

exports.Domain = Domain;

exports.create = exports.createDomain = function(cb) {
return new Domain(cb);
};

// it's possible to enter one domain while already inside
// another one. the stack is each entered domain.
var stack = [];
exports._stack = stack;
// the active domain is always the one that we're currently in.
exports.active = null;

Expand Down
76 changes: 69 additions & 7 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ Persistent<String> domain_symbol;
// declared in node_internals.h
Persistent<Object> process;

static Persistent<Array> domains_stack;

static Persistent<Function> process_tickFromSpinner;
static Persistent<Function> process_tickCallback;

Expand All @@ -127,6 +129,8 @@ static Persistent<String> exit_symbol;
static Persistent<String> disposed_symbol;

static Persistent<String> emitting_toplevel_domain_error_symbol;
static Persistent<String> _events_symbol;
static Persistent<String> error_symbol;

static bool print_eval = false;
static bool force_repl = false;
Expand Down Expand Up @@ -905,25 +909,77 @@ Handle<Value> FromConstructorTemplate(Persistent<FunctionTemplate> t,
return scope.Close(t->GetFunction()->NewInstance(argc, argv));
}

static bool IsDomainActive() {
if (domain_symbol.IsEmpty())
domain_symbol = NODE_PSYMBOL("domain");
static bool DomainHasErrorHandler(const Local<Object>& domain) {
HandleScope scope;

if (_events_symbol.IsEmpty())
_events_symbol = NODE_PSYMBOL("_events");

Local<Value> domain_event_listeners_v = domain->Get(_events_symbol);
if (!domain_event_listeners_v->IsObject())
return false;

Local<Object> domain_event_listeners_o =
domain_event_listeners_v.As<Object>();

if (error_symbol.IsEmpty())
error_symbol = NODE_PSYMBOL("error");

Local<Value> domain_error_listeners_v =
domain_event_listeners_o->Get(error_symbol);

if (domain_error_listeners_v->IsFunction() ||
(domain_error_listeners_v->IsArray() &&
domain_error_listeners_v.As<Array>()->Length() > 0))
return true;

return false;
}

static bool TopDomainHasErrorHandler() {
HandleScope scope;

if (!using_domains)
return false;

Local<Value> domain_v = process->Get(domain_symbol);
Local<Array> domains_stack_array = Local<Array>::New(domains_stack);
if (domains_stack_array->Length() == 0)
return false;

return domain_v->IsObject();
uint32_t domains_stack_length = domains_stack_array->Length();
if (domains_stack_length == 0)
return false;

Local<Value> domain_v = domains_stack_array->Get(domains_stack_length - 1);
if (!domain_v->IsObject())
return false;

Local<Object> domain = domain_v.As<Object>();
if (DomainHasErrorHandler(domain))
return true;

return false;
}

bool ShouldAbortOnUncaughtException() {
Local<Value> emitting_toplevel_domain_error_v =
process->Get(emitting_toplevel_domain_error_symbol);
return !IsDomainActive() || emitting_toplevel_domain_error_v->BooleanValue();

return emitting_toplevel_domain_error_v->BooleanValue() ||
!TopDomainHasErrorHandler();
}

Handle<Value> UsingDomains(const Arguments& args) {
HandleScope scope;

if (using_domains)
return scope.Close(Undefined());

if (!args[0]->IsArray()) {
fprintf(stderr, "domains stack must be an array\n");
abort();
}

using_domains = true;
Local<Value> tdc_v = process->Get(String::New("_tickDomainCallback"));
Local<Value> ndt_v = process->Get(String::New("_nextDomainTick"));
Expand All @@ -935,6 +991,9 @@ Handle<Value> UsingDomains(const Arguments& args) {
fprintf(stderr, "process._nextDomainTick assigned to non-function\n");
abort();
}

domains_stack = Persistent<Array>::New(args[0].As<Array>());

Local<Function> tdc = tdc_v.As<Function>();
Local<Function> ndt = ndt_v.As<Function>();
process->Set(String::New("_tickCallback"), tdc);
Expand Down Expand Up @@ -2460,7 +2519,10 @@ Handle<Object> SetupProcessObject(int argc, char *argv[]) {
process->Set(String::NewSymbol("_tickInfoBox"), info_box);

// pre-set _events object for faster emit checks
process->Set(String::NewSymbol("_events"), Object::New());
if (_events_symbol.IsEmpty())
_events_symbol = NODE_PSYMBOL("_events");

process->Set(_events_symbol, Object::New());

if (emitting_toplevel_domain_error_symbol.IsEmpty())
emitting_toplevel_domain_error_symbol =
Expand Down
22 changes: 16 additions & 6 deletions src/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@
// and exit if there are no listeners.
process._fatalException = function(er) {
var caught = false;

if (process.domain) {
var domain = process.domain;
var domainModule = NativeModule.require('domain');
Expand Down Expand Up @@ -256,11 +257,17 @@
// aborting in these cases.
if (domainStack.length === 1) {
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 = domain.emit('error', er);
// If there's no error handler, do not emit an 'error' event
// as this would throw an error, make the process exit, and thus
// prevent the process 'uncaughtException' event from being emitted
// if a listener is set.
if (process.EventEmitter.listenerCount(domain, 'error') > 0) {
// 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 = domain.emit('error', er);
}
} finally {
process._emittingTopLevelDomainError = false;
}
Expand Down Expand Up @@ -297,9 +304,12 @@
// current tick and no domains should be left on the stack
// between ticks.
_clearDomainsStack();
} else {
}

if (!caught) {
caught = 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
if (!caught) {
Expand Down
34 changes: 34 additions & 0 deletions test/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,3 +226,37 @@ exports.busyLoop = function busyLoop(time) {
;
}
};

// Returns true if the exit code "exitCode" and/or signal name "signal"
// represent the exit code and/or signal name of a node process that aborted,
// false otherwise.
exports.nodeProcessAborted = function nodeProcessAborted(exitCode, signal) {
// Depending on the compiler used, node will exit with either
// exit code 132 (SIGILL), 133 (SIGTRAP) or 134 (SIGABRT).
var expectedExitCodes = [132, 133, 134];

// On platforms using KSH as the default shell (like SmartOS),
// when a process aborts, KSH exits with an exit code that is
// greater than 256, and thus the exit code emitted with the 'exit'
// event is null and the signal is set to either SIGILL, SIGTRAP,
// or SIGABRT (depending on the compiler).
var expectedSignals = ['SIGILL', 'SIGTRAP', 'SIGABRT'];

// On Windows, v8's base::OS::Abort triggers a debug breakpoint,
// which makes the process exit with code -2147483645.
if (process.platform === 'win32')
expectedExitCodes = [-2147483645];

// When using --abort-on-uncaught-exception, V8 will use
// base::OS::Abort to terminate the process.
// Depending on the compiler used, the shell or other aspects of
// the platform used to build the node binary, this will actually
// make V8 exit by aborting or by raising a signal. In any case,
// one of them (exit code or signal) needs to be set to one of
// the expected exit codes or signals.
if (signal !== null) {
return expectedSignals.indexOf(signal) > -1;
} else {
return expectedExitCodes.indexOf(exitCode) > -1;
}
};
Loading

0 comments on commit 563c359

Please sign in to comment.