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.

Finally, change the behavior of --abort-on-uncaught-exception so that,
if the domain within which the error is thrown has no error handler, but
a domain further up the domains stack has one, the process will not
abort.

Fixes nodejs#3607 and nodejs#3653.

PR: nodejs#3654
PR-URL: nodejs#3654
Reviewed-By: Chris Dickinson <chris@neversaw.us>
  • Loading branch information
Julien Gilli authored and Michael Scovetta committed Apr 2, 2016
1 parent 0a50e40 commit 3a5eb84
Show file tree
Hide file tree
Showing 9 changed files with 808 additions and 126 deletions.
33 changes: 20 additions & 13 deletions lib/domain.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,20 @@ Object.defineProperty(process, 'domain', {
}
});

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

// let the process know we're using domains
const _domain_flag = process._setupDomainUse(_domain);
const _domain_flag = process._setupDomainUse(_domain, stack);

exports.Domain = Domain;

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

// 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 Expand Up @@ -96,14 +97,20 @@ Domain.prototype._errorHandler = function errorHandler(er) {
// that these exceptions are caught, and thus would prevent it from
// aborting in these cases.
if (stack.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 = emitError();
} finally {
process._emittingTopLevelDomainError = false;
// 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 (EventEmitter.listenerCount(self, 'error') > 0) {
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 = emitError();
} finally {
process._emittingTopLevelDomainError = false;
}
}
} else {
// wrap this in a try/catch so we don't get infinite throwing
Expand Down
1 change: 1 addition & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ namespace node {
V(buffer_prototype_object, v8::Object) \
V(context, v8::Context) \
V(domain_array, v8::Array) \
V(domains_stack_array, v8::Array) \
V(fs_stats_constructor_function, v8::Function) \
V(generic_internal_field_template, v8::ObjectTemplate) \
V(jsstream_constructor_template, v8::FunctionTemplate) \
Expand Down
48 changes: 42 additions & 6 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -951,17 +951,50 @@ void* ArrayBufferAllocator::Allocate(size_t size) {
return malloc(size);
}

static bool DomainHasErrorHandler(const Environment* env,
const Local<Object>& domain) {
HandleScope scope(env->isolate());

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

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

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

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 DomainsStackHasErrorHandler(const Environment* env) {
HandleScope scope(env->isolate());

static bool IsDomainActive(const Environment* env) {
if (!env->using_domains())
return false;

Local<Array> domain_array = env->domain_array().As<Array>();
if (domain_array->Length() == 0)
Local<Array> domains_stack_array = env->domains_stack_array().As<Array>();
if (domains_stack_array->Length() == 0)
return false;

Local<Value> domain_v = domain_array->Get(0);
return !domain_v->IsNull();
uint32_t domains_stack_length = domains_stack_array->Length();
for (uint32_t i = domains_stack_length; i > 0; --i) {
Local<Value> domain_v = domains_stack_array->Get(i - 1);
if (!domain_v->IsObject())
return false;

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

return false;
}


Expand All @@ -975,7 +1008,7 @@ static bool ShouldAbortOnUncaughtException(Isolate* isolate) {
bool isEmittingTopLevelDomainError =
process_object->Get(emitting_top_level_domain_error_key)->BooleanValue();

return !IsDomainActive(env) || isEmittingTopLevelDomainError;
return isEmittingTopLevelDomainError || !DomainsStackHasErrorHandler(env);
}


Expand Down Expand Up @@ -1004,6 +1037,9 @@ void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
CHECK(args[0]->IsArray());
env->set_domain_array(args[0].As<Array>());

CHECK(args[1]->IsArray());
env->set_domains_stack_array(args[1].As<Array>());

// Do a little housekeeping.
env->process_object()->Delete(
FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupDomainUse"));
Expand Down
34 changes: 34 additions & 0 deletions test/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -471,3 +471,37 @@ ArrayStream.prototype.writable = true;
ArrayStream.prototype.pause = function() {};
ArrayStream.prototype.resume = function() {};
ArrayStream.prototype.write = function() {};

// 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).
const expectedSignals = ['SIGILL', 'SIGTRAP', 'SIGABRT'];

// On Windows, v8's base::OS::Abort triggers an access violation,
// which corresponds to exit code 3221225477 (0xC0000005)
if (process.platform === 'win32')
expectedExitCodes = [3221225477];

// 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 3a5eb84

Please sign in to comment.