Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

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: #8631
Fixes: #8630
PR-URL: #8666
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
  • Loading branch information
Julien Gilli authored and trevnorris committed Nov 19, 2014
1 parent fbff705 commit caeb677
Show file tree
Hide file tree
Showing 4 changed files with 364 additions and 31 deletions.
21 changes: 21 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,20 @@ 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);

return domain_v->IsObject();
}

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 +2452,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 +2979,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->SetAbortOnUncaughtException(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 caeb677

Please sign in to comment.