domain: Do not use uncaughtException handler

isaacs · isaacs · commit 4401bb47bfa4 · 2012-12-29T10:37:30.000-08:00
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
diff --git 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
@@ -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
@@ -110,9 +110,7 @@ static Persistent<String> rss_symbol;
 static Persistent<String> heap_total_symbol;
 static Persistent<String> heap_used_symbol;
 
-static Persistent<String> listeners_symbol;
-static Persistent<String> uncaught_exception_symbol;
-static Persistent<String> emit_symbol;
+static Persistent<String> fatal_exception_symbol;
 
 static Persistent<Function> 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<Value> listeners_v = process->Get(listeners_symbol);
-  assert(listeners_v->IsFunction());
-
-  Local<Function> listeners = Local<Function>::Cast(listeners_v);
-
-  Local<String> uncaught_exception_symbol_l = Local<String>::New(uncaught_exception_symbol);
-  Local<Value> argv[1] = { uncaught_exception_symbol_l  };
-  Local<Value> ret = listeners->Call(process, 1, argv);
+  if (fatal_exception_symbol.IsEmpty())
+    fatal_exception_symbol = NODE_PSYMBOL("_fatalException");
 
-  assert(ret->IsArray());
+  Local<Value> fatal_v = process->Get(fatal_exception_symbol);
 
-  Local<Array> listener_array = Local<Array>::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<Value> emit_v = process->Get(emit_symbol);
-  assert(emit_v->IsFunction());
-
-  Local<Function> emit = Local<Function>::Cast(emit_v);
+  Local<Function> fatal_f = Local<Function>::Cast(fatal_v);
 
   Local<Value> error = try_catch.Exception();
-  Local<Value> event_argv[2] = { uncaught_exception_symbol_l, error };
+  Local<Value> argv[] = { error };
+
+  TryCatch fatal_try_catch;
+
+  // this will return true if the JS layer handled it, false otherwise
+  Local<Value> 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
@@ -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
@@ -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++;
