Skip to content

Commit

Permalink
domains: fix issues with abort on uncaught
Browse files Browse the repository at this point in the history
Do not abort the process if an error is thrown from within a domain, an
error handler is setup for the domain and --abort-on-uncaught-exception
was passed on the command line.

However, if an error is thrown from within the top-level domain's error
handler and --abort-on-uncaught-exception was passed on the command
line, make the process abort.

Fixes nodejs#8631. Also fixes nodejs#8630.
  • Loading branch information
Julien Gilli committed Nov 3, 2014
1 parent 523929c commit fd61ee3
Show file tree
Hide file tree
Showing 8 changed files with 364 additions and 34 deletions.
3 changes: 2 additions & 1 deletion deps/v8/include/v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -2856,7 +2856,8 @@ class V8EXPORT Isolate {
* restored when exiting. Re-entering an isolate is allowed.
*/
void Enter();

typedef bool (*should_abort_on_uncaught_exception_handler_t)();
void SetShouldAbortOnUncaughtExceptionHandler(should_abort_on_uncaught_exception_handler_t handler);
/**
* Exits this isolate by restoring the previously entered one in the
* current thread. The isolate may still stay the same, if it was
Expand Down
4 changes: 4 additions & 0 deletions deps/v8/src/api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5565,6 +5565,10 @@ void Isolate::Enter() {
isolate->Enter();
}

void Isolate::SetShouldAbortOnUncaughtExceptionHandler(should_abort_on_uncaught_exception_handler_t handler) {
i::Isolate* isolate = reinterpret_cast<i::Isolate*>(this);
isolate->SetShouldAbortOnUncaughtExceptionHandler(handler);
}

void Isolate::Exit() {
i::Isolate* isolate = reinterpret_cast<i::Isolate*>(this);
Expand Down
9 changes: 7 additions & 2 deletions deps/v8/src/isolate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1158,7 +1158,8 @@ void Isolate::DoThrow(Object* exception, MessageLocation* location) {
// print a user-friendly stack trace (not an internal one).
if (fatal_exception_depth == 0 &&
FLAG_abort_on_uncaught_exception &&
(report_exception || can_be_caught_externally)) {
(report_exception || can_be_caught_externally) &&
(!should_abort_on_uncaught_exception_handler_ || should_abort_on_uncaught_exception_handler_())) {
fatal_exception_depth++;
fprintf(stderr, "%s\n\nFROM\n",
*MessageHandler::GetLocalizedMessage(message_obj));
Expand Down Expand Up @@ -1339,6 +1340,9 @@ void Isolate::SetCaptureStackTraceForUncaughtExceptions(
stack_trace_for_uncaught_exceptions_options_ = options;
}

void Isolate::SetShouldAbortOnUncaughtExceptionHandler(v8::Isolate::should_abort_on_uncaught_exception_handler_t handler) {
should_abort_on_uncaught_exception_handler_ = handler;
}

bool Isolate::is_out_of_memory() {
if (has_pending_exception()) {
Expand Down Expand Up @@ -1534,7 +1538,8 @@ Isolate::Isolate()
date_cache_(NULL),
context_exit_happened_(false),
deferred_handles_head_(NULL),
optimizing_compiler_thread_(this) {
optimizing_compiler_thread_(this),
should_abort_on_uncaught_exception_handler_(0) {
TRACE_ISOLATE(constructor);

memset(isolate_addresses_, 0,
Expand Down
5 changes: 5 additions & 0 deletions deps/v8/src/isolate.h
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,9 @@ class Isolate {
int frame_limit,
StackTrace::StackTraceOptions options);

typedef bool (*should_abort_on_uncaught_exception_handler_t)();
void SetShouldAbortOnUncaughtExceptionHandler(should_abort_on_uncaught_exception_handler_t handler);

// Tells whether the current context has experienced an out of memory
// exception.
bool is_out_of_memory();
Expand Down Expand Up @@ -1292,6 +1295,8 @@ class Isolate {
DeferredHandles* deferred_handles_head_;
OptimizingCompilerThread optimizing_compiler_thread_;

should_abort_on_uncaught_exception_handler_t should_abort_on_uncaught_exception_handler_;

friend class ExecutionAccess;
friend class HandleScopeImplementer;
friend class IsolateInitializer;
Expand Down
30 changes: 30 additions & 0 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ static Persistent<String> enter_symbol;
static Persistent<String> exit_symbol;
static Persistent<String> disposed_symbol;

static Persistent<String> emitting_toplevel_domain_error_symbol;

static bool print_eval = false;
static bool force_repl = false;
Expand Down Expand Up @@ -904,6 +905,29 @@ 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");

Local<Value> domain_v = process->Get(domain_symbol);

bool has_domain = domain_v->IsObject();
if (has_domain) {
Local<Object> domain = domain_v->ToObject();
assert(!domain.IsEmpty());
if (!domain->IsNull()) {
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();
}

Handle<Value> UsingDomains(const Arguments& args) {
HandleScope scope;
Expand Down Expand Up @@ -2437,6 +2461,11 @@ Handle<Object> SetupProcessObject(int argc, char *argv[]) {
// pre-set _events object for faster emit checks
process->Set(String::NewSymbol("_events"), Object::New());

if (emitting_toplevel_domain_error_symbol.IsEmpty())
emitting_toplevel_domain_error_symbol =
NODE_PSYMBOL("_emittingTopLevelDomainError");
process->Set(emitting_toplevel_domain_error_symbol, False());

return process;
}

Expand Down Expand Up @@ -2959,6 +2988,7 @@ char** Init(int argc, char *argv[]) {
// Fetch a reference to the main isolate, so we have a reference to it
// even when we need it to access it from another (debugger) thread.
node_isolate = Isolate::GetCurrent();
node_isolate->SetShouldAbortOnUncaughtExceptionHandler(ShouldAbortOnUncaughtException);

// If the --debug flag was specified then initialize the debug thread.
if (use_debug_agent) {
Expand Down
85 changes: 54 additions & 31 deletions src/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -236,37 +236,60 @@

er.domain = domain;
er.domainThrown = true;
// wrap this in a try/catch so we don't get infinite throwing
try {
// One of three things will happen here.
//
// 1. There is a handler, caught = true
// 2. There is no handler, caught = false
// 3. It throws, caught = false
//
// If caught is false after this, then there's no need to exit()
// the domain, because we're going to crash the process anyway.
caught = domain.emit('error', er);

// Exit all domains on the stack. Uncaught exceptions end the
// current tick and no domains should be left on the stack
// between ticks.
var domainModule = NativeModule.require('domain');
domainStack.length = 0;
domainModule.active = process.domain = null;
} catch (er2) {
// The domain error handler threw! oh no!
// See if another domain can catch THIS error,
// or else crash on the original one.
// If the user already exited it, then don't double-exit.
if (domain === domainModule.active)
domainStack.pop();
if (domainStack.length) {
var parentDomain = domainStack[domainStack.length - 1];
process.domain = domainModule.active = parentDomain;
caught = process._fatalException(er2);
} else
caught = false;

// The top-level domain-handler is handled separately.
//
// The reason is that if V8 was passed a command line option
// asking it to abort on an uncaught exception (currently
// "--abort-on-uncaught-exception"), we want an uncaught exception
// in the top-level domain error handler to make the
// process abort. Using try/catch here would always make V8 think
// that these exceptions are caught, and thus would prevent it from
// 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);
} finally {
process._emittingTopLevelDomainError = false;
}
} else {
// wrap this in a try/catch so we don't get infinite throwing
try {
// One of three things will happen here.
//
// 1. There is a handler, caught = true
// 2. There is no handler, caught = false
// 3. It throws, caught = false
//
// If caught is false after this, then there's no need to exit()
// the domain, because we're going to crash the process anyway.
caught = domain.emit('error', er);

// Exit all domains on the stack. Uncaught exceptions end the
// current tick and no domains should be left on the stack
// between ticks.
var domainModule = NativeModule.require('domain');
domainStack.length = 0;
domainModule.active = process.domain = null;
} catch (er2) {
// The domain error handler threw! oh no!
// See if another domain can catch THIS error,
// or else crash on the original one.
// If the user already exited it, then don't double-exit.
if (domain === domainModule.active)
domainStack.pop();
if (domainStack.length) {
var parentDomain = domainStack[domainStack.length - 1];
process.domain = domainModule.active = parentDomain;
caught = process._fatalException(er2);
} else {
caught = false;
}
}
}
} else {
caught = process.emit('uncaughtException', er);
Expand Down
74 changes: 74 additions & 0 deletions test/simple/test-domain-top-level-error-handler-throw.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright Joyent, Inc. and other Node contributors.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

/*
* The goal of this test is to make sure that when a top-level error
* handler throws an error following the handling of a previous error,
* the process reports the error message from the error thrown in the
* top-level error handler, not the one from the previous error.
*/

var domainErrHandlerExMessage = 'exception from domain error handler';
var internalExMessage = 'You should NOT see me';

if (process.argv[2] === 'child') {
var domain = require('domain');
var d = domain.create();

d.on('error', function() {
throw new Error(domainErrHandlerExMessage);
});

d.run(function doStuff() {
process.nextTick(function () {
throw new Error(internalExMessage);
});
});
} else {
var fork = require('child_process').fork;
var assert = require('assert');

function test() {
var child = fork(process.argv[1], ['child'], {silent:true});
var gotDataFromStderr = false;
var stderrOutput = '';
if (child) {
child.stderr.on('data', function onStderrData(data) {
gotDataFromStderr = true;
stderrOutput += data.toString();
})

child.on('exit', function onChildExited(exitCode, signal) {
assert(gotDataFromStderr);
assert(stderrOutput.indexOf(domainErrHandlerExMessage) !== -1);
assert(stderrOutput.indexOf(internalExMessage) === -1);

var expectedExitCode = 7;
var expectedSignal = null;

assert.equal(exitCode, expectedExitCode);
assert.equal(signal, expectedSignal);
});
}
}

test();
}
Loading

0 comments on commit fd61ee3

Please sign in to comment.