From 4401bb47bfa4fe72c2755c428577903ece5cfaa0 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 26 Dec 2012 12:28:33 -0800 Subject: [PATCH] domain: Do not use uncaughtException handler This adds a process._fatalException method which is called into from C++ in order to either emit the 'uncaughtException' method, or emit 'error' on the active domain. The 'uncaughtException' event is an implementation detail that it would be nice to deprecate one day, so exposing it as part of the domain machinery is not ideal. Fix #4375 --- lib/domain.js | 21 ---------- lib/timers.js | 29 ++++++++------ src/node.cc | 55 ++++++++++--------------- src/node.js | 44 ++++++++++++++++++++ test/simple/test-domain.js | 82 ++++++++++++++++++++++---------------- 5 files changed, 129 insertions(+), 102 deletions(-) diff --git a/lib/domain.js b/lib/domain.js index 4fefb913c8..32259d7a73 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -46,27 +46,6 @@ exports._stack = stack; exports.active = null; -// loading this file the first time sets up the global -// uncaughtException handler. -process.on('uncaughtException', uncaughtHandler); - -function uncaughtHandler(er) { - // if there's an active domain, then handle this there. - // Note that if this error emission throws, then it'll just crash. - if (exports.active && !exports.active._disposed) { - util._extend(er, { - domain: exports.active, - domain_thrown: true - }); - exports.active.emit('error', er); - if (exports.active) exports.active.exit(); - } else if (process.listeners('uncaughtException').length === 1) { - // if there are other handlers, then they'll take care of it. - // but if not, then we need to crash now. - throw er; - } -} - inherits(Domain, EventEmitter); function Domain() { diff --git a/lib/timers.js b/lib/timers.js index 48353c04ef..8ec6b2c852 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -86,23 +86,28 @@ function insert(item, msecs) { if (!first._onTimeout) continue; - // v0.4 compatibility: if the timer callback throws and the user's - // uncaughtException handler ignores the exception, other timers that - // expire on this tick should still run. If #2582 goes through, this - // hack should be removed. + // v0.4 compatibility: if the timer callback throws and the + // domain or uncaughtException handler ignore the exception, + // other timers that expire on this tick should still run. // // https://github.com/joyent/node/issues/2631 - if (first.domain) { - if (first.domain._disposed) continue; - first.domain.enter(); - } + var domain = first.domain; + if (domain && domain._disposed) continue; try { + if (domain) + domain.enter(); + var threw = true; first._onTimeout(); - } catch (e) { - if (!process.listeners('uncaughtException').length) throw e; - process.emit('uncaughtException', e); + if (domain) + domain.exit(); + threw = false; + } finally { + if (threw) { + process.nextTick(function() { + list.ontimeout(); + }); + } } - if (first.domain) first.domain.exit(); } } diff --git a/src/node.cc b/src/node.cc index 3e8d2d6158..3829268d77 100644 --- a/src/node.cc +++ b/src/node.cc @@ -110,9 +110,7 @@ static Persistent rss_symbol; static Persistent heap_total_symbol; static Persistent heap_used_symbol; -static Persistent listeners_symbol; -static Persistent uncaught_exception_symbol; -static Persistent emit_symbol; +static Persistent fatal_exception_symbol; static Persistent process_makeCallback; @@ -1883,47 +1881,36 @@ static void OnFatalError(const char* location, const char* message) { void FatalException(TryCatch &try_catch) { HandleScope scope; - if (listeners_symbol.IsEmpty()) { - listeners_symbol = NODE_PSYMBOL("listeners"); - uncaught_exception_symbol = NODE_PSYMBOL("uncaughtException"); - emit_symbol = NODE_PSYMBOL("emit"); - } - - Local listeners_v = process->Get(listeners_symbol); - assert(listeners_v->IsFunction()); - - Local listeners = Local::Cast(listeners_v); - - Local uncaught_exception_symbol_l = Local::New(uncaught_exception_symbol); - Local argv[1] = { uncaught_exception_symbol_l }; - Local ret = listeners->Call(process, 1, argv); + if (fatal_exception_symbol.IsEmpty()) + fatal_exception_symbol = NODE_PSYMBOL("_fatalException"); - assert(ret->IsArray()); + Local fatal_v = process->Get(fatal_exception_symbol); - Local listener_array = Local::Cast(ret); - - uint32_t length = listener_array->Length(); - // Report and exit if process has no "uncaughtException" listener - if (length == 0) { + if (!fatal_v->IsFunction()) { + // failed before the process._fatalException function was added! + // this is probably pretty bad. Nothing to do but report and exit. ReportException(try_catch, true); exit(1); } - // Otherwise fire the process "uncaughtException" event - Local emit_v = process->Get(emit_symbol); - assert(emit_v->IsFunction()); - - Local emit = Local::Cast(emit_v); + Local fatal_f = Local::Cast(fatal_v); Local error = try_catch.Exception(); - Local event_argv[2] = { uncaught_exception_symbol_l, error }; + Local argv[] = { error }; + + TryCatch fatal_try_catch; + + // this will return true if the JS layer handled it, false otherwise + Local caught = fatal_f->Call(process, ARRAY_SIZE(argv), argv); - TryCatch event_try_catch; - emit->Call(process, 2, event_argv); + if (fatal_try_catch.HasCaught()) { + // the fatal exception function threw, so we must exit + ReportException(fatal_try_catch, true); + exit(1); + } - if (event_try_catch.HasCaught()) { - // the uncaught exception event threw, so we must exit. - ReportException(event_try_catch, true); + if (false == caught->BooleanValue()) { + ReportException(try_catch, true); exit(1); } } diff --git a/src/node.js b/src/node.js index 6cce75f68e..bfd312b30f 100644 --- a/src/node.js +++ b/src/node.js @@ -38,6 +38,9 @@ process.EventEmitter = EventEmitter; // process.EventEmitter is deprecated + // do this good and early, since it handles errors. + startup.processFatal(); + startup.globalVariables(); startup.globalTimeouts(); startup.globalConsole(); @@ -211,6 +214,47 @@ return startup._lazyConstants; }; + startup.processFatal = function() { + // call into the active domain, or emit uncaughtException, + // and exit if there are no listeners. + process._fatalException = function(er) { + var caught = false; + if (process.domain) { + var domain = process.domain; + + // ignore errors on disposed domains. + // + // XXX This is a bit stupid. We should probably get rid of + // domain.dispose() altogether. It's almost always a terrible + // idea. --isaacs + if (domain._disposed) + return true; + + er.domain = domain; + er.domain_thrown = 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); + domain.exit(); + } catch (er2) { + caught = false; + } + } else { + caught = process.emit('uncaughtException', er); + } + // if someone handled it, then great. otherwise, die in C++ land + return caught; + }; + }; + var assert; startup.processAssert = function() { // Note that calls to assert() are pre-processed out by JS2C for the diff --git a/test/simple/test-domain.js b/test/simple/test-domain.js index a478c1e91d..183e2deb92 100644 --- a/test/simple/test-domain.js +++ b/test/simple/test-domain.js @@ -26,14 +26,15 @@ var common = require('../common'); var assert = require('assert'); var domain = require('domain'); var events = require('events'); +var fs = require('fs'); var caught = 0; -var expectCaught = 8; +var expectCaught = 0; var d = new domain.Domain(); var e = new events.EventEmitter(); d.on('error', function(er) { - console.error('caught', er); + console.error('caught', er && (er.message || er)); var er_message = er.message; var er_path = er.path @@ -110,24 +111,58 @@ d.on('error', function(er) { break; default: - console.error('unexpected error, throwing %j', er.message); + console.error('unexpected error, throwing %j', er.message || er); throw er; } caught++; }); + + process.on('exit', function() { - console.error('exit'); - assert.equal(caught, expectCaught); + console.error('exit', caught, expectCaught); + assert.equal(caught, expectCaught, 'caught the expected number of errors'); console.log('ok'); }); +// catch thrown errors no matter how many times we enter the event loop +// this only uses implicit binding, except for the first function +// passed to d.run(). The rest are implicitly bound by virtue of being +// set up while in the scope of the d domain. +d.run(function() { + process.nextTick(function() { + var i = setInterval(function () { + clearInterval(i); + setTimeout(function() { + fs.stat('this file does not exist', function(er, stat) { + // uh oh! stat isn't set! + // pretty common error. + console.log(stat.isDirectory()); + }); + }); + }); + }); +}); +expectCaught++; + + + +// implicit addition of a timer created within a domain-bound context. +d.run(function() { + setTimeout(function() { + throw new Error('implicit timer'); + }); +}); +expectCaught++; + + // Event emitters added to the domain have their errors routed. d.add(e); e.emit('error', new Error('emitted')); +expectCaught++; @@ -141,6 +176,9 @@ function fn(er) { var bound = d.intercept(fn); bound(new Error('bound')); +expectCaught++; + + // intercepted should never pass first argument to callback function fn2(data) { @@ -169,36 +207,16 @@ function thrower() { throw new Error('thrown'); } setTimeout(d.bind(thrower), 100); +expectCaught++; // Pass an intercepted function to an fs operation that fails. -var fs = require('fs'); fs.open('this file does not exist', 'r', d.intercept(function(er) { console.error('should not get here!', er); throw new Error('should not get here!'); }, true)); - - - -// catch thrown errors no matter how many times we enter the event loop -// this only uses implicit binding, except for the first function -// passed to d.run(). The rest are implicitly bound by virtue of being -// set up while in the scope of the d domain. -d.run(function() { - process.nextTick(function() { - var i = setInterval(function () { - clearInterval(i); - setTimeout(function() { - fs.stat('this file does not exist', function(er, stat) { - // uh oh! stat isn't set! - // pretty common error. - console.log(stat.isDirectory()); - }); - }); - }); - }); -}); +expectCaught++; @@ -213,16 +231,9 @@ setTimeout(function() { // escape from the domain, but implicit is still bound to it. implicit.emit('error', new Error('implicit')); }, 10); +expectCaught++; - -// implicit addition of a timer created within a domain-bound context. -d.run(function() { - setTimeout(function() { - throw new Error('implicit timer'); - }); -}); - var result = d.run(function () { return 'return value'; }); @@ -231,3 +242,4 @@ assert.equal(result, 'return value'); var fst = fs.createReadStream('stream for nonexistent file') d.add(fst) +expectCaught++;