Skip to content

Commit 425a354

Browse files
author
Julien Gilli
committed
domains: fix handling of uncaught exceptions
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 #3607 and #3653. PR: #3654 PR-URL: #3654 Reviewed-By: Chris Dickinson <[email protected]>
1 parent 1a21a53 commit 425a354

9 files changed

+808
-126
lines changed

lib/domain.js

+20-13
Original file line numberDiff line numberDiff line change
@@ -27,19 +27,20 @@ Object.defineProperty(process, 'domain', {
2727
}
2828
});
2929

30+
// It's possible to enter one domain while already inside
31+
// another one. The stack is each entered domain.
32+
const stack = [];
33+
exports._stack = stack;
34+
3035
// let the process know we're using domains
31-
const _domain_flag = process._setupDomainUse(_domain);
36+
const _domain_flag = process._setupDomainUse(_domain, stack);
3237

3338
exports.Domain = Domain;
3439

3540
exports.create = exports.createDomain = function() {
3641
return new Domain();
3742
};
3843

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

@@ -96,14 +97,20 @@ Domain.prototype._errorHandler = function errorHandler(er) {
9697
// that these exceptions are caught, and thus would prevent it from
9798
// aborting in these cases.
9899
if (stack.length === 1) {
99-
try {
100-
// Set the _emittingTopLevelDomainError so that we know that, even
101-
// if technically the top-level domain is still active, it would
102-
// be ok to abort on an uncaught exception at this point
103-
process._emittingTopLevelDomainError = true;
104-
caught = emitError();
105-
} finally {
106-
process._emittingTopLevelDomainError = false;
100+
// If there's no error handler, do not emit an 'error' event
101+
// as this would throw an error, make the process exit, and thus
102+
// prevent the process 'uncaughtException' event from being emitted
103+
// if a listener is set.
104+
if (EventEmitter.listenerCount(self, 'error') > 0) {
105+
try {
106+
// Set the _emittingTopLevelDomainError so that we know that, even
107+
// if technically the top-level domain is still active, it would
108+
// be ok to abort on an uncaught exception at this point
109+
process._emittingTopLevelDomainError = true;
110+
caught = emitError();
111+
} finally {
112+
process._emittingTopLevelDomainError = false;
113+
}
107114
}
108115
} else {
109116
// wrap this in a try/catch so we don't get infinite throwing

src/env.h

+1
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,7 @@ namespace node {
242242
V(buffer_prototype_object, v8::Object) \
243243
V(context, v8::Context) \
244244
V(domain_array, v8::Array) \
245+
V(domains_stack_array, v8::Array) \
245246
V(fs_stats_constructor_function, v8::Function) \
246247
V(generic_internal_field_template, v8::ObjectTemplate) \
247248
V(jsstream_constructor_template, v8::FunctionTemplate) \

src/node.cc

+42-6
Original file line numberDiff line numberDiff line change
@@ -951,17 +951,50 @@ void* ArrayBufferAllocator::Allocate(size_t size) {
951951
return malloc(size);
952952
}
953953

954+
static bool DomainHasErrorHandler(const Environment* env,
955+
const Local<Object>& domain) {
956+
HandleScope scope(env->isolate());
957+
958+
Local<Value> domain_event_listeners_v = domain->Get(env->events_string());
959+
if (!domain_event_listeners_v->IsObject())
960+
return false;
961+
962+
Local<Object> domain_event_listeners_o =
963+
domain_event_listeners_v.As<Object>();
964+
965+
Local<Value> domain_error_listeners_v =
966+
domain_event_listeners_o->Get(env->error_string());
967+
968+
if (domain_error_listeners_v->IsFunction() ||
969+
(domain_error_listeners_v->IsArray() &&
970+
domain_error_listeners_v.As<Array>()->Length() > 0))
971+
return true;
972+
973+
return false;
974+
}
975+
976+
static bool DomainsStackHasErrorHandler(const Environment* env) {
977+
HandleScope scope(env->isolate());
954978

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

959-
Local<Array> domain_array = env->domain_array().As<Array>();
960-
if (domain_array->Length() == 0)
982+
Local<Array> domains_stack_array = env->domains_stack_array().As<Array>();
983+
if (domains_stack_array->Length() == 0)
961984
return false;
962985

963-
Local<Value> domain_v = domain_array->Get(0);
964-
return !domain_v->IsNull();
986+
uint32_t domains_stack_length = domains_stack_array->Length();
987+
for (uint32_t i = domains_stack_length; i > 0; --i) {
988+
Local<Value> domain_v = domains_stack_array->Get(i - 1);
989+
if (!domain_v->IsObject())
990+
return false;
991+
992+
Local<Object> domain = domain_v.As<Object>();
993+
if (DomainHasErrorHandler(env, domain))
994+
return true;
995+
}
996+
997+
return false;
965998
}
966999

9671000

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

978-
return !IsDomainActive(env) || isEmittingTopLevelDomainError;
1011+
return isEmittingTopLevelDomainError || !DomainsStackHasErrorHandler(env);
9791012
}
9801013

9811014

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

1040+
CHECK(args[1]->IsArray());
1041+
env->set_domains_stack_array(args[1].As<Array>());
1042+
10071043
// Do a little housekeeping.
10081044
env->process_object()->Delete(
10091045
FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupDomainUse"));

test/common.js

+34
Original file line numberDiff line numberDiff line change
@@ -471,3 +471,37 @@ ArrayStream.prototype.writable = true;
471471
ArrayStream.prototype.pause = function() {};
472472
ArrayStream.prototype.resume = function() {};
473473
ArrayStream.prototype.write = function() {};
474+
475+
// Returns true if the exit code "exitCode" and/or signal name "signal"
476+
// represent the exit code and/or signal name of a node process that aborted,
477+
// false otherwise.
478+
exports.nodeProcessAborted = function nodeProcessAborted(exitCode, signal) {
479+
// Depending on the compiler used, node will exit with either
480+
// exit code 132 (SIGILL), 133 (SIGTRAP) or 134 (SIGABRT).
481+
var expectedExitCodes = [132, 133, 134];
482+
483+
// On platforms using KSH as the default shell (like SmartOS),
484+
// when a process aborts, KSH exits with an exit code that is
485+
// greater than 256, and thus the exit code emitted with the 'exit'
486+
// event is null and the signal is set to either SIGILL, SIGTRAP,
487+
// or SIGABRT (depending on the compiler).
488+
const expectedSignals = ['SIGILL', 'SIGTRAP', 'SIGABRT'];
489+
490+
// On Windows, v8's base::OS::Abort triggers an access violation,
491+
// which corresponds to exit code 3221225477 (0xC0000005)
492+
if (process.platform === 'win32')
493+
expectedExitCodes = [3221225477];
494+
495+
// When using --abort-on-uncaught-exception, V8 will use
496+
// base::OS::Abort to terminate the process.
497+
// Depending on the compiler used, the shell or other aspects of
498+
// the platform used to build the node binary, this will actually
499+
// make V8 exit by aborting or by raising a signal. In any case,
500+
// one of them (exit code or signal) needs to be set to one of
501+
// the expected exit codes or signals.
502+
if (signal !== null) {
503+
return expectedSignals.indexOf(signal) > -1;
504+
} else {
505+
return expectedExitCodes.indexOf(exitCode) > -1;
506+
}
507+
};

0 commit comments

Comments
 (0)