Skip to content

Commit

Permalink
src: fix --abort-on-uncaught-exception
Browse files Browse the repository at this point in the history
Revert 0af4c9e, parts of
921f2de and port
nodejs/node-v0.x-archive#25835 from v0.12 to
master so that node aborts at the right time when an error is thrown
and --abort-on-uncaught-exception is used.

Fixes #3035.

PR: #3036
PR-URL: #3036
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
  • Loading branch information
whitlockjc authored and jasnell committed Oct 8, 2015
1 parent 8b50e95 commit 546e833
Show file tree
Hide file tree
Showing 8 changed files with 363 additions and 78 deletions.
88 changes: 59 additions & 29 deletions lib/domain.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,20 @@ Domain.prototype._disposed = undefined;
// Called by process._fatalException in case an error was thrown.
Domain.prototype._errorHandler = function errorHandler(er) {
var caught = false;
var self = this;

function emitError() {
var handled = self.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.
stack.length = 0;
exports.active = process.domain = null;

return handled;
}

// ignore errors on disposed domains.
//
// XXX This is a bit stupid. We should probably get rid of
Expand All @@ -71,38 +85,54 @@ Domain.prototype._errorHandler = function errorHandler(er) {
er.domain = this;
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 = this.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.
stack.length = 0;
exports.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 (this === exports.active) {
stack.pop();
// 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 (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 (stack.length) {
exports.active = process.domain = stack[stack.length - 1];
caught = process._fatalException(er2);
} else {
caught = 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 = emitError();
} 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 (this === exports.active) {
stack.pop();
}
if (stack.length) {
exports.active = process.domain = stack[stack.length - 1];
caught = process._fatalException(er2);
} else {
caught = false;
}
return caught;
}
return caught;
}
return caught;
};
Expand Down
18 changes: 1 addition & 17 deletions src/async-wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,6 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
Local<Object> process = env()->process_object();
Local<Object> domain;
bool has_domain = false;
bool has_abort_on_uncaught_and_domains = false;

if (env()->using_domains()) {
Local<Value> domain_v = context->Get(env()->domain_string());
Expand All @@ -177,7 +176,6 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
domain = domain_v.As<Object>();
if (domain->Get(env()->disposed_string())->IsTrue())
return Undefined(env()->isolate());
has_abort_on_uncaught_and_domains = env()->using_abort_on_uncaught_exc();
}
}

Expand All @@ -201,21 +199,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
try_catch.SetVerbose(true);
}

Local<Value> ret;

if (has_abort_on_uncaught_and_domains) {
Local<Value> fn = process->Get(env()->domain_abort_uncaught_exc_string());
if (fn->IsFunction()) {
Local<Array> special_context = Array::New(env()->isolate(), 2);
special_context->Set(0, context);
special_context->Set(1, cb);
ret = fn.As<Function>()->Call(special_context, argc, argv);
} else {
ret = cb->Call(context, argc, argv);
}
} else {
ret = cb->Call(context, argc, argv);
}
Local<Value> ret = cb->Call(context, argc, argv);

if (try_catch.HasCaught()) {
return Undefined(env()->isolate());
Expand Down
9 changes: 0 additions & 9 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,6 @@ inline Environment::Environment(v8::Local<v8::Context> context,
isolate_data_(IsolateData::GetOrCreate(context->GetIsolate(), loop)),
timer_base_(uv_now(loop)),
using_domains_(false),
using_abort_on_uncaught_exc_(false),
using_asyncwrap_(false),
printed_error_(false),
trace_sync_io_(false),
Expand Down Expand Up @@ -341,14 +340,6 @@ inline uint64_t Environment::timer_base() const {
return timer_base_;
}

inline bool Environment::using_abort_on_uncaught_exc() const {
return using_abort_on_uncaught_exc_;
}

inline void Environment::set_using_abort_on_uncaught_exc(bool value) {
using_abort_on_uncaught_exc_ = value;
}

inline bool Environment::using_domains() const {
return using_domains_;
}
Expand Down
6 changes: 1 addition & 5 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ namespace node {
V(dev_string, "dev") \
V(disposed_string, "_disposed") \
V(domain_string, "domain") \
V(domain_abort_uncaught_exc_string, "_makeCallbackAbortOnUncaught") \
V(emitting_top_level_domain_error_string, "_emittingTopLevelDomainError") \
V(exchange_string, "exchange") \
V(idle_string, "idle") \
V(irq_string, "irq") \
Expand Down Expand Up @@ -432,9 +432,6 @@ class Environment {
inline ares_channel* cares_channel_ptr();
inline ares_task_list* cares_task_list();

inline bool using_abort_on_uncaught_exc() const;
inline void set_using_abort_on_uncaught_exc(bool value);

inline bool using_domains() const;
inline void set_using_domains(bool value);

Expand Down Expand Up @@ -540,7 +537,6 @@ class Environment {
ares_channel cares_channel_;
ares_task_list cares_task_list_;
bool using_domains_;
bool using_abort_on_uncaught_exc_;
bool using_asyncwrap_;
bool printed_error_;
bool trace_sync_io_;
Expand Down
43 changes: 32 additions & 11 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ static bool force_repl = false;
static bool syntax_check_only = false;
static bool trace_deprecation = false;
static bool throw_deprecation = false;
static bool abort_on_uncaught_exception = false;
static bool trace_sync_io = false;
static bool track_heap_objects = false;
static const char* eval_string = nullptr;
Expand Down Expand Up @@ -907,6 +906,33 @@ void* ArrayBufferAllocator::Allocate(size_t size) {
}


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)
return false;

Local<Value> domain_v = domain_array->Get(0);
return !domain_v->IsNull();
}


static bool ShouldAbortOnUncaughtException(Isolate* isolate) {
HandleScope scope(isolate);

Environment* env = Environment::GetCurrent(isolate);
Local<Object> process_object = env->process_object();
Local<String> emitting_top_level_domain_error_key =
env->emitting_top_level_domain_error_string();
bool isEmittingTopLevelDomainError =
process_object->Get(emitting_top_level_domain_error_key)->BooleanValue();

return !IsDomainActive(env) || isEmittingTopLevelDomainError;
}


void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand Down Expand Up @@ -2197,11 +2223,7 @@ void FatalException(Isolate* isolate,

if (false == caught->BooleanValue()) {
ReportException(env, error, message);
if (abort_on_uncaught_exception) {
ABORT();
} else {
exit(1);
}
exit(1);
}
}

Expand Down Expand Up @@ -3229,9 +3251,6 @@ static void ParseArgs(int* argc,
track_heap_objects = true;
} else if (strcmp(arg, "--throw-deprecation") == 0) {
throw_deprecation = true;
} else if (strcmp(arg, "--abort-on-uncaught-exception") == 0 ||
strcmp(arg, "--abort_on_uncaught_exception") == 0) {
abort_on_uncaught_exception = true;
} else if (strcmp(arg, "--v8-options") == 0) {
new_v8_argv[new_v8_argc] = "--help";
new_v8_argc += 1;
Expand Down Expand Up @@ -3935,8 +3954,10 @@ static void StartNodeInstance(void* arg) {
Environment* env = CreateEnvironment(isolate, context, instance_data);
array_buffer_allocator->set_env(env);
Context::Scope context_scope(context);
if (instance_data->is_main())
env->set_using_abort_on_uncaught_exc(abort_on_uncaught_exception);

node_isolate->SetAbortOnUncaughtExceptionCallback(
ShouldAbortOnUncaughtException);

// Start debug agent when argv has --debug
if (instance_data->use_debug_agent())
StartDebug(env, debug_wait_connect);
Expand Down
7 changes: 0 additions & 7 deletions src/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,13 +210,6 @@
};

startup.processFatal = function() {
process._makeCallbackAbortOnUncaught = function() {
try {
return this[1].apply(this[0], arguments);
} catch (err) {
process._fatalException(err);
}
};

process._fatalException = function(er) {
var caught;
Expand Down
50 changes: 50 additions & 0 deletions test/parallel/test-domain-top-level-error-handler-throw.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
'use strict';

/*
* 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.
*/

const common = require('../common');

const domainErrHandlerExMessage = 'exception from domain error handler';
const 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');

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

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

var expectedExitCode = 7;
var expectedSignal = null;

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

0 comments on commit 546e833

Please sign in to comment.