Skip to content

Commit

Permalink
src: fix abort-on-uncaught-exception
Browse files Browse the repository at this point in the history 
This PR fixes 0af4c9e so that node
aborts at the right time when throwing an error and using
--abort-on-uncaught-exception.

Basically, it wraps most node internal callbacks with:

if (!domain || domain.emittingTopLevelError)
  runCallback();
else {
  try {
    runCallback();
  } catch (err) {
    process._fatalException(err);
  }
}

so that V8 can abort properly in Isolate::Throw if
--abort-on-uncaught-exception was passed on the command line, and domain
can handle the error if one is active and not already in the top level
domain's error handler.

It also reverts 921f2de partially:
node::FatalException does not abort anymore because at that time, it's
already too late.

It adds process._forceTickDone, which is really a hack to allow
test-next-tick-error-spin.js to pass. It's here to basically avoid an
infinite recursion when throwing in a domain from a nextTick callback,
and queuing the same callback on the next tick from the domain's error
handler.

This change is an alternative approach to nodejs#3036 for fixing nodejs#3035.

Fixes nodejs#3035.
  • Loading branch information
Julien Gilli committed Sep 24, 2015
1 parent 02448c6 commit ee8b68a
Show file tree
Hide file tree
Showing 9 changed files with 504 additions and 108 deletions.
92 changes: 58 additions & 34 deletions lib/domain.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,39 +71,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();

if (stack.length === 1) {
try {
this.emittingTopLevelError = true;

caught = this.emit('error', er);

stack.length = 0;
exports.active = process.domain = null;
} finally {
this.emittingTopLevelError = 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 = 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();
}
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 Expand Up @@ -176,20 +191,29 @@ Domain.prototype.run = function(fn) {
if (this._disposed)
return;

var ret;
var ret, args;

this.enter();
if (arguments.length >= 2) {
var len = arguments.length;
var args = new Array(len - 1);
args = new Array(len - 1);

for (var i = 1; i < len; i++)
args[i - 1] = arguments[i];
}

// Wrap this in a try/catch block so that
//
// domain.run(function() { throw new Error("foo"); })
//
// actually emits an error event on the domain.
try {
ret = fn.apply(this, args);
} else {
ret = fn.call(this);
} catch (err) {
process._forceTickDone();
process._fatalException(err);
}

this.exit();

return ret;
Expand Down
18 changes: 4 additions & 14 deletions lib/repl.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,20 +159,10 @@ function REPLServer(prompt,
regExMatcher.test(savedRegExMatches.join(sep));

if (!err) {
try {
if (self.useGlobal) {
result = script.runInThisContext({ displayErrors: false });
} else {
result = script.runInContext(context, { displayErrors: false });
}
} catch (e) {
err = e;
if (err && process.domain) {
debug('not recoverable, send to domain');
process.domain.emit('error', err);
process.domain.exit();
return;
}
if (self.useGlobal) {
result = script.runInThisContext({ displayErrors: false });
} else {
result = script.runInContext(context, { displayErrors: false });
}
}

Expand Down
112 changes: 80 additions & 32 deletions lib/timers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,24 +81,30 @@ function listOnTimeout() {
if (domain && domain._disposed)
continue;

try {
if (domain)
domain.enter();
threw = true;
first._called = true;
first._onTimeout();
if (domain)
domain.exit();
threw = false;
} finally {
if (threw) {
// We need to continue processing after domain error handling
// is complete, but not by using whatever domain was left over
// when the timeout threw its exception.
var oldDomain = process.domain;
process.domain = null;
process.nextTick(listOnTimeoutNT, list);
process.domain = oldDomain;
// If there's no active domain or we're in the
// error handler for the domain at the top of the stack,
// don't use try/catch to allow --abort-on-uncaught-exception
// to abort if an error is thrown.
// Otherwise, use try/catch to get the chance to emit an
// error on the current domain
if (!domain || domain.emittingTopLevelError) {
try {
fireTimer(first);
threw = false;
} finally {
if (threw) {
postFireTimer();
}
}
} else {
try {
fireTimer(first);
threw = false;
} catch (err) {
process._fatalException(err);
if (threw) {
postFireTimer();
}
}
}
}
Expand All @@ -108,6 +114,26 @@ function listOnTimeout() {
assert(L.isEmpty(list));
list.close();
delete lists[msecs];

function fireTimer(timer) {
if (domain)
domain.enter();
threw = true;
first._called = true;
first._onTimeout();
if (domain)
domain.exit();
}

function postFireTimer() {
// We need to continue processing after domain error handling
// is complete, but not by using whatever domain was left over
// when the timeout threw its exception.
var oldDomain = process.domain;
process.domain = null;
process.nextTick(listOnTimeoutNT, list);
process.domain = oldDomain;
}
}


Expand Down Expand Up @@ -370,20 +396,28 @@ function processImmediate() {
domain.enter();

var threw = true;
try {
immediate._onImmediate();
threw = false;
} finally {
if (threw) {
if (!L.isEmpty(queue)) {
// Handle any remaining on next tick, assuming we're still
// alive to do so.
while (!L.isEmpty(immediateQueue)) {
L.append(queue, L.shift(immediateQueue));
}
immediateQueue = queue;
process.nextTick(processImmediate);
}

// If there's no active domain or we're in the
// error handler for the domain at the top of the stack,
// don't use try/catch to allow --abort-on-uncaught-exception
// to abort if an error is thrown.
// Otherwise, use try/catch to get the chance to emit an
// error on the current domain
if (!domain || domain.emittingTopLevelError) {
try {
immediate._onImmediate();
threw = false;
} finally {
postImmediate(threw);
}
} else {
try {
immediate._onImmediate();
threw = false;
} catch (err) {
process._fatalException(err);

postImmediate(threw);
}
}

Expand All @@ -397,6 +431,20 @@ function processImmediate() {
if (L.isEmpty(immediateQueue)) {
process._needImmediateCallback = false;
}

function postImmediate(threw) {
if (threw) {
if (!L.isEmpty(queue)) {
// Handle any remaining on next tick, assuming we're still
// alive to do so.
while (!L.isEmpty(immediateQueue)) {
L.append(queue, L.shift(immediateQueue));
}
immediateQueue = queue;
process.nextTick(processImmediate);
}
}
}
}


Expand Down
10 changes: 5 additions & 5 deletions src/node.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2180,11 +2180,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 @@ -3201,6 +3197,10 @@ static void ParseArgs(int* argc,
} else if (strcmp(arg, "--abort-on-uncaught-exception") == 0 ||
strcmp(arg, "--abort_on_uncaught_exception") == 0) {
abort_on_uncaught_exception = true;
// Pass this option to V8 anyway, because V8 uses it to determine
// if it needs to abort on an uncaught exception
new_v8_argv[new_v8_argc] = arg;
new_v8_argc += 1;
} else if (strcmp(arg, "--v8-options") == 0) {
new_v8_argv[new_v8_argc] = "--help";
new_v8_argc += 1;
Expand Down
Loading

0 comments on commit ee8b68a

Please sign in to comment.