process: streamlining tick callback logic

* Callbacks from spinner now calls its own function, separate from the
  tickCallback logic
* MakeCallback will call a domain specific function if a domain is
  detected
* _tickCallback assumes no domains, until nextTick receives a callback
  with a domain. After that _tickCallback is overridden with the domain
  specific implementation.
* _needTickCallback runs in startup() instead of nextTick (isaacs)
* Fix bug in _fatalException where exit would be called twice (isaacs)
* Process.domain has a default value of null
* Manually track nextTickQueue.length (will be useful later)
* Update tests to reflect internal api changes

src/node.cc

@@ -100,6 +100,8 @@ Persistent<String> process_symbol;
 Persistent<String> domain_symbol;
 
 static Persistent<Object> process;
+static Persistent<Function> process_tickWDCallback;
+static Persistent<Function> process_tickFromSpinner;
 static Persistent<Function> process_tickCallback;
 
 static Persistent<String> exports_symbol;
@@ -174,20 +176,19 @@ static void Tick(void) {
 
   HandleScope scope;
 
-  if (tick_callback_sym.IsEmpty()) {
-    // Lazily set the symbol
-    tick_callback_sym = NODE_PSYMBOL("_tickCallback");
+  if (process_tickFromSpinner.IsEmpty()) {
+    Local<Value> cb_v = process->Get(String::New("_tickFromSpinner"));
+    if (!cb_v->IsFunction()) {
+      fprintf(stderr, "process._tickFromSpinner assigned to non-function\n");
+      abort();
+    }
+    Local<Function> cb = cb_v.As<Function>();
+    process_tickFromSpinner = Persistent<Function>::New(cb);
   }
 
-  Local<Value> cb_v = process->Get(tick_callback_sym);
-  if (!cb_v->IsFunction()) return;
-  Local<Function> cb = Local<Function>::Cast(cb_v);
-
   TryCatch try_catch;
 
-  // Let the tick callback know that this is coming from the spinner
-  Handle<Value> argv[] = { True(node_isolate) };
-  cb->Call(process, ARRAY_SIZE(argv), argv);
+  process_tickFromSpinner->Call(process, 0, NULL);
 
   if (try_catch.HasCaught()) {
     FatalException(try_catch);
@@ -903,25 +904,79 @@ Handle<Value> FromConstructorTemplate(Persistent<FunctionTemplate> t,
 }
 
 
-// MakeCallback may only be made directly off the event loop.
-// That is there can be no JavaScript stack frames underneath it.
-// (Is there any way to assert that?)
-//
-// Maybe make this a method of a node::Handle super class
-//
 Handle<Value>
-MakeCallback(const Handle<Object> object,
-             const char* method,
+MCWithDomain(const Handle<Object> object,
+             const Handle<Function> callback,
              int argc,
              Handle<Value> argv[]) {
-  HandleScope scope;
+  // lazy load _tickWDCallback
+  if (process_tickWDCallback.IsEmpty()) {
+    Local<Value> cb_v = process->Get(String::New("_tickWDCallback"));
+    if (!cb_v->IsFunction()) {
+      fprintf(stderr, "process._tickWDCallback assigned to non-function\n");
+      abort();
+    }
+    Local<Function> cb = cb_v.As<Function>();
+    process_tickWDCallback = Persistent<Function>::New(cb);
+  }
 
-  Handle<Value> ret =
-    MakeCallback(object, String::NewSymbol(method), argc, argv);
+  // lazy load domain specific symbols
+  if (enter_symbol.IsEmpty()) {
+    enter_symbol = NODE_PSYMBOL("enter");
+    exit_symbol = NODE_PSYMBOL("exit");
+    disposed_symbol = NODE_PSYMBOL("_disposed");
+  }
 
-  return scope.Close(ret);
+  Local<Value> domain_v = object->Get(domain_symbol);
+  Local<Object> domain;
+  Local<Function> enter;
+  Local<Function> exit;
+
+  TryCatch try_catch;
+
+  domain = domain_v->ToObject();
+  assert(!domain.IsEmpty());
+  if (domain->Get(disposed_symbol)->IsTrue()) {
+    // domain has been disposed of.
+    return Undefined(node_isolate);
+  }
+  enter = Local<Function>::Cast(domain->Get(enter_symbol));
+  assert(!enter.IsEmpty());
+  enter->Call(domain, 0, NULL);
+
+  if (try_catch.HasCaught()) {
+    FatalException(try_catch);
+    return Undefined(node_isolate);
+  }
+
+  Local<Value> ret = callback->Call(object, argc, argv);
+
+  if (try_catch.HasCaught()) {
+    FatalException(try_catch);
+    return Undefined(node_isolate);
+  }
+
+  exit = Local<Function>::Cast(domain->Get(exit_symbol));
+  assert(!exit.IsEmpty());
+  exit->Call(domain, 0, NULL);
+
+  if (try_catch.HasCaught()) {
+    FatalException(try_catch);
+    return Undefined(node_isolate);
+  }
+
+  // process nextTicks after call
+  process_tickWDCallback->Call(process, 0, NULL);
+
+  if (try_catch.HasCaught()) {
+    FatalException(try_catch);
+    return Undefined(node_isolate);
+  }
+
+  return ret;
 }
 
+
 Handle<Value>
 MakeCallback(const Handle<Object> object,
              const Handle<String> symbol,
@@ -932,61 +987,15 @@ MakeCallback(const Handle<Object> object,
   Local<Value> callback_v = object->Get(symbol);
   if (!callback_v->IsFunction()) {
     String::Utf8Value method(symbol);
-    // XXX: If the object has a domain attached, handle it there?
-    // At least, would be good to get *some* sort of indication
-    // of how we got here, even if it's not catchable.
     fprintf(stderr, "Non-function in MakeCallback. method = %s\n", *method);
     abort();
   }
 
+  Local<Function> callback = Local<Function>::Cast(callback_v);
+
   // TODO Hook for long stack traces to be made here.
 
-  TryCatch try_catch;
-
-  if (enter_symbol.IsEmpty()) {
-    enter_symbol = NODE_PSYMBOL("enter");
-    exit_symbol = NODE_PSYMBOL("exit");
-    disposed_symbol = NODE_PSYMBOL("_disposed");
-  }
-
-  Local<Value> domain_v = object->Get(domain_symbol);
-  Local<Object> domain;
-  Local<Function> enter;
-  Local<Function> exit;
-  if (!domain_v->IsUndefined()) {
-    domain = domain_v->ToObject();
-    if (domain->Get(disposed_symbol)->BooleanValue()) {
-      // domain has been disposed of.
-      return Undefined(node_isolate);
-    }
-    enter = Local<Function>::Cast(domain->Get(enter_symbol));
-    enter->Call(domain, 0, NULL);
-  }
-
-  if (try_catch.HasCaught()) {
-    FatalException(try_catch);
-    return Undefined(node_isolate);
-  }
-
-  Local<Function> callback = Local<Function>::Cast(callback_v);
-  Local<Value> ret = callback->Call(object, argc, argv);
-
-  if (try_catch.HasCaught()) {
-    FatalException(try_catch);
-    return Undefined(node_isolate);
-  }
-
-  if (!domain_v->IsUndefined()) {
-    exit = Local<Function>::Cast(domain->Get(exit_symbol));
-    exit->Call(domain, 0, NULL);
-  }
-
-  if (try_catch.HasCaught()) {
-    FatalException(try_catch);
-    return Undefined(node_isolate);
-  }
-
-  // process nextTicks after every time we get called.
+  // lazy load no domain next tick callbacks
   if (process_tickCallback.IsEmpty()) {
     Local<Value> cb_v = process->Get(String::New("_tickCallback"));
     if (!cb_v->IsFunction()) {
@@ -996,13 +1005,44 @@ MakeCallback(const Handle<Object> object,
     Local<Function> cb = cb_v.As<Function>();
     process_tickCallback = Persistent<Function>::New(cb);
   }
-  process_tickCallback->Call(process, NULL, 0);
+
+  // has domain, off with you
+  if (object->Has(domain_symbol) &&
+      !object->Get(domain_symbol)->IsNull() &&
+      !object->Get(domain_symbol)->IsUndefined())
+    return scope.Close(MCWithDomain(object, callback, argc, argv));
+
+  TryCatch try_catch;
+
+  Local<Value> ret = callback->Call(object, argc, argv);
 
   if (try_catch.HasCaught()) {
     FatalException(try_catch);
     return Undefined(node_isolate);
   }
 
+  // process nextTicks after call
+  process_tickCallback->Call(process, 0, NULL);
+
+  if (try_catch.HasCaught()) {
+    FatalException(try_catch);
+    return Undefined(node_isolate);
+  }
+
+  return scope.Close(ret);
+}
+
+
+Handle<Value>
+MakeCallback(const Handle<Object> object,
+             const char* method,
+             int argc,
+             Handle<Value> argv[]) {
+  HandleScope scope;
+
+  Handle<Value> ret =
+    MakeCallback(object, String::NewSymbol(method), argc, argv);
+
   return scope.Close(ret);
 }
 

src/node.js

@@ -156,6 +156,8 @@
         });
       }
     }
+
+    process._needTickCallback();
   }
 
   startup.globalVariables = function() {
@@ -165,6 +167,8 @@
     global.root = global;
     global.Buffer = NativeModule.require('buffer').Buffer;
     process.binding('buffer').setFastBufferConstructor(global.Buffer);
+    process.domain = null;
+    process._exiting = false;
   };
 
   startup.globalTimeouts = function() {
@@ -262,11 +266,17 @@
       // since that means that we'll exit the process, emit the 'exit' event
       if (!caught) {
         try {
-          process.emit('exit', 1);
+          if (!process._exiting) {
+            process._exiting = true;
+            process.emit('exit', 1);
+          }
         } catch (er) {
           // nothing to be done about it at this point.
         }
       }
+      // if we handled an error, then make sure any ticks get processed
+      if (caught)
+        process._needTickCallback();
       return caught;
     };
   };
@@ -297,10 +307,20 @@
   };
 
   startup.processNextTick = function() {
+    var _needTickCallback = process._needTickCallback;
     var nextTickQueue = [];
+    var nextTickQueueLength = 0;
     var nextTickIndex = 0;
-    var inTick = false;
     var tickDepth = 0;
+    var usingDomains = false;
+    var needSpinner = true;
+    var inTick = false;
+
+    process._tickCallback = _tickCallback;
+    process._tickFromSpinner = _tickFromSpinner;
+    // needs to be accessible from cc land
+    process._tickWDCallback = _tickWDCallback;
+    process.nextTick = nextTick;
 
     // the maximum number of times it'll process something like
     // nextTick(function f(){nextTick(f)})
@@ -312,13 +332,22 @@
     process.maxTickDepth = 1000;
 
     function tickDone(tickDepth_) {
-      tickDepth = tickDepth_ || 0;
-      nextTickQueue.splice(0, nextTickIndex);
-      nextTickIndex = 0;
-      inTick = false;
-      if (nextTickQueue.length) {
-        process._needTickCallback();
+      if (nextTickQueueLength !== 0) {
+        if (nextTickQueueLength <= nextTickIndex) {
+          nextTickQueue = [];
+          nextTickQueueLength = 0;
+        } else {
+          nextTickQueue.splice(0, nextTickIndex);
+          nextTickQueueLength = nextTickQueue.length;
+          if (needSpinner) {
+            _needTickCallback();
+            needSpinner = false;
+          }
+        }
       }
+      inTick = false;
+      nextTickIndex = 0;
+      tickDepth = tickDepth_;
     }
 
     function maxTickWarn() {
@@ -332,26 +361,65 @@
         console.error(msg);
     }
 
-    process._tickCallback = function(fromSpinner) {
+    function _tickFromSpinner() {
+      needSpinner = true;
+      // coming from spinner, reset!
+      if (tickDepth !== 0)
+        tickDepth = 0;
+      // no callbacks to run
+      if (nextTickQueueLength === 0)
+        return nextTickIndex = tickDepth = 0;
+      if (nextTickQueue[nextTickQueueLength - 1].domain)
+        _tickWDCallback();
+      else
+        _tickCallback();
+    }
+
+    // run callback that have no domain
+    // this is very hot, so has been super optimized
+    // this will be overridden if user uses domains
+    function _tickCallback() {
+      var callback, nextTickLength, threw;
+
+      if (inTick) return;
+      if (nextTickQueueLength === 0) {
+        nextTickIndex = 0;
+        tickDepth = 0;
+        return;
+      }
+      inTick = true;
+
+      while (tickDepth++ < process.maxTickDepth) {
+        nextTickLength = nextTickQueueLength;
+        if (nextTickIndex === nextTickLength)
+          return tickDone(0);
+
+        while (nextTickIndex < nextTickLength) {
+          callback = nextTickQueue[nextTickIndex++].callback;
+          threw = true;
+          try {
+            callback();
+            threw = false;
+          } finally {
+            if (threw) tickDone(tickDepth);
+          }
+        }
+      }
+
+      tickDone(0);
+    }
+
+    function _tickWDCallback() {
+      var nextTickLength, tock, callback, threw;
+
       // if you add a nextTick in a domain's error handler, then
       // it's possible to cycle indefinitely.  Normally, the tickDone
       // in the finally{} block below will prevent this, however if
       // that error handler ALSO triggers multiple MakeCallbacks, then
       // it'll try to keep clearing the queue, since the finally block
       // fires *before* the error hits the top level and is handled.
-      if (tickDepth >= process.maxTickDepth) {
-        if (fromSpinner) {
-          // coming in from the event queue.  reset.
-          tickDepth = 0;
-        } else {
-          if (nextTickQueue.length) {
-            process._needTickCallback();
-          }
-          return;
-        }
-      }
-
-      if (!nextTickQueue.length) return tickDone();
+      if (tickDepth >= process.maxTickDepth)
+        return _needTickCallback();
 
       if (inTick) return;
       inTick = true;
@@ -360,18 +428,19 @@
       // is set to some negative value, or if there were repeated errors
       // preventing tickDepth from being cleared, we'd never process any
       // of them.
-      do {
-        tickDepth++;
-        var nextTickLength = nextTickQueue.length;
-        if (nextTickLength === 0) return tickDone();
+      while (tickDepth++ < process.maxTickDepth) {
+        nextTickLength = nextTickQueueLength;
+        if (nextTickIndex === nextTickLength)
+          return tickDone(0);
+
         while (nextTickIndex < nextTickLength) {
-          var tock = nextTickQueue[nextTickIndex++];
-          var callback = tock.callback;
+          tock = nextTickQueue[nextTickIndex++];
+          callback = tock.callback;
           if (tock.domain) {
             if (tock.domain._disposed) continue;
             tock.domain.enter();
           }
-          var threw = true;
+          threw = true;
           try {
             callback();
             threw = false;
@@ -384,31 +453,31 @@
             tock.domain.exit();
           }
         }
-        nextTickQueue.splice(0, nextTickIndex);
-        nextTickIndex = 0;
+      }
 
-        // continue until the max depth or we run out of tocks.
-      } while (tickDepth < process.maxTickDepth &&
-               nextTickQueue.length > 0);
-
-      tickDone();
-    };
-
-    process.nextTick = function(callback) {
-      // on the way out, don't bother.
-      // it won't get fired anyway.
-      if (process._exiting) return;
+      tickDone(0);
+    }
 
+    function nextTick(callback) {
+      // on the way out, don't bother. it won't get fired anyway.
+      if (process._exiting)
+        return;
       if (tickDepth >= process.maxTickDepth)
         maxTickWarn();
 
-      var tock = { callback: callback };
-      if (process.domain) tock.domain = process.domain;
-      nextTickQueue.push(tock);
-      if (nextTickQueue.length) {
-        process._needTickCallback();
+      var obj = { callback: callback };
+      if (process.domain !== null) {
+        obj.domain = process.domain;
+        // user has opt'd to use domains, so override default functionality
+        if (!usingDomains) {
+          process._tickCallback = _tickWDCallback;
+          usingDomains = true;
+        }
       }
-    };
+
+      nextTickQueue.push(obj);
+      nextTickQueueLength++;
+    }
   };
 
   function evalScript(name) {

test/message/error_exit.out

@@ -10,4 +10,5 @@ AssertionError: 1 == 2
     at Module.load (module.js:*:*)
     at Function.Module._load (module.js:*:*)
     at Module.runMain (module.js:*:*)
-    at process._tickCallback (node.js:*:*)
+    at _tickCallback (node.js:*:*)
+    at process._tickFromSpinner (node.js:*:*)

test/message/max_tick_depth_trace.out

@@ -11,7 +11,8 @@ Trace: (node) warning: Recursive process.nextTick detected. This will break in t
     at maxTickWarn (node.js:*:*)
     at process.nextTick (node.js:*:*
     at f (*test*message*max_tick_depth_trace.js:*:*)
-    at process._tickCallback (node.js:*:*)
+    at _tickCallback (node.js:*:*)
+    at process._tickFromSpinner (node.js:*:*)
 tick 11
 tick 10
 tick 9
@@ -26,6 +27,7 @@ Trace: (node) warning: Recursive process.nextTick detected. This will break in t
     at maxTickWarn (node.js:*:*)
     at process.nextTick (node.js:*:*
     at f (*test*message*max_tick_depth_trace.js:*:*)
-    at process._tickCallback (node.js:*:*)
+    at _tickCallback (node.js:*:*)
+    at process._tickFromSpinner (node.js:*:*)
 tick 1
 tick 0

test/message/nexttick_throw.out

@@ -3,4 +3,5 @@
         ^
 ReferenceError: undefined_reference_error_maker is not defined
     at *test*message*nexttick_throw.js:*:*
-    at process._tickCallback (node.js:*:*)
+    at _tickCallback (node.js:*:*)
+    at process._tickFromSpinner (node.js:*:*)

test/message/undefined_reference_in_new_context.out

@@ -11,4 +11,5 @@ ReferenceError: foo is not defined
     at Module.load (module.js:*)
     at *._load (module.js:*)
     at Module.runMain (module.js:*)
-    at *._tickCallback (node.js:*)
+    at _tickCallback (node.js:*)
+    at *._tickFromSpinner (node.js:*)

test/simple/test-timers-uncaught-exception.js

@@ -27,24 +27,32 @@ var timer1 = 0;
 var timer2 = 0;
 
 // the first timer throws...
+console.error('set first timer');
 setTimeout(function() {
+  console.error('first timer');
   timer1++;
   throw new Error('BAM!');
 }, 100);
 
 // ...but the second one should still run
+console.error('set second timer');
 setTimeout(function() {
+  console.error('second timer');
   assert.equal(timer1, 1);
   timer2++;
 }, 100);
 
 function uncaughtException(err) {
+  console.error('uncaught handler');
   assert.equal(err.message, 'BAM!');
   exceptions++;
 }
 process.on('uncaughtException', uncaughtException);
 
+var exited = false;
 process.on('exit', function() {
+  assert(!exited);
+  exited = true;
   process.removeListener('uncaughtException', uncaughtException);
   assert.equal(exceptions, 1);
   assert.equal(timer1, 1);