Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 425a354

Browse filesBrowse 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 <chris@neversaw.us>
1 parent 1a21a53 commit 425a354
Copy full SHA for 425a354
Expand file treeCollapse file tree

9 files changed

+808
-126
lines changed
Open diff view settings
Collapse file

‎lib/domain.js‎

Copy file name to clipboardExpand all lines: lib/domain.js
+20-13Lines changed: 20 additions & 13 deletions
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
Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
+1Lines changed: 1 addition & 0 deletions
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) \
Collapse file

‎src/node.cc‎

Copy file name to clipboardExpand all lines: src/node.cc
+42-6Lines changed: 42 additions & 6 deletions
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"));
Collapse file

‎test/common.js‎

Copy file name to clipboardExpand all lines: test/common.js
+34Lines changed: 34 additions & 0 deletions
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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.