From 86c0745a5ebd75a4a5fa69420dfde657a5e717bc Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 6 Feb 2013 17:26:18 -0800 Subject: [PATCH] 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 | 184 +++++++++++------- src/node.js | 165 +++++++++++----- test/message/error_exit.out | 3 +- test/message/max_tick_depth_trace.out | 6 +- test/message/nexttick_throw.out | 3 +- .../undefined_reference_in_new_context.out | 3 +- test/simple/test-timers-uncaught-exception.js | 8 + 7 files changed, 247 insertions(+), 125 deletions(-) diff --git a/src/node.cc b/src/node.cc index 890ea6355de..c4131069e52 100644 --- a/src/node.cc +++ b/src/node.cc @@ -100,6 +100,8 @@ Persistent process_symbol; Persistent domain_symbol; static Persistent process; +static Persistent process_tickWDCallback; +static Persistent process_tickFromSpinner; static Persistent process_tickCallback; static Persistent 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 cb_v = process->Get(String::New("_tickFromSpinner")); + if (!cb_v->IsFunction()) { + fprintf(stderr, "process._tickFromSpinner assigned to non-function\n"); + abort(); + } + Local cb = cb_v.As(); + process_tickFromSpinner = Persistent::New(cb); } - Local cb_v = process->Get(tick_callback_sym); - if (!cb_v->IsFunction()) return; - Local cb = Local::Cast(cb_v); - TryCatch try_catch; - // Let the tick callback know that this is coming from the spinner - Handle 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 FromConstructorTemplate(Persistent 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 -MakeCallback(const Handle object, - const char* method, +MCWithDomain(const Handle object, + const Handle callback, int argc, Handle argv[]) { - HandleScope scope; + // lazy load _tickWDCallback + if (process_tickWDCallback.IsEmpty()) { + Local cb_v = process->Get(String::New("_tickWDCallback")); + if (!cb_v->IsFunction()) { + fprintf(stderr, "process._tickWDCallback assigned to non-function\n"); + abort(); + } + Local cb = cb_v.As(); + process_tickWDCallback = Persistent::New(cb); + } - Handle 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 domain_v = object->Get(domain_symbol); + Local domain; + Local enter; + Local 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::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 ret = callback->Call(object, argc, argv); + + if (try_catch.HasCaught()) { + FatalException(try_catch); + return Undefined(node_isolate); + } + + exit = Local::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 MakeCallback(const Handle object, const Handle symbol, @@ -932,61 +987,15 @@ MakeCallback(const Handle object, Local 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 callback = Local::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 domain_v = object->Get(domain_symbol); - Local domain; - Local enter; - Local 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::Cast(domain->Get(enter_symbol)); - enter->Call(domain, 0, NULL); - } - - if (try_catch.HasCaught()) { - FatalException(try_catch); - return Undefined(node_isolate); - } - - Local callback = Local::Cast(callback_v); - Local ret = callback->Call(object, argc, argv); - - if (try_catch.HasCaught()) { - FatalException(try_catch); - return Undefined(node_isolate); - } - - if (!domain_v->IsUndefined()) { - exit = Local::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 cb_v = process->Get(String::New("_tickCallback")); if (!cb_v->IsFunction()) { @@ -996,13 +1005,44 @@ MakeCallback(const Handle object, Local cb = cb_v.As(); process_tickCallback = Persistent::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 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 +MakeCallback(const Handle object, + const char* method, + int argc, + Handle argv[]) { + HandleScope scope; + + Handle ret = + MakeCallback(object, String::NewSymbol(method), argc, argv); + return scope.Close(ret); } diff --git a/src/node.js b/src/node.js index a9e795acdd4..bc2ff0a292c 100644 --- a/src/node.js +++ b/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) { diff --git a/test/message/error_exit.out b/test/message/error_exit.out index 0e01e12567a..acdd424ca93 100644 --- a/test/message/error_exit.out +++ b/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:*:*) diff --git a/test/message/max_tick_depth_trace.out b/test/message/max_tick_depth_trace.out index 0fab6cd2c7f..180e5c90f43 100644 --- a/test/message/max_tick_depth_trace.out +++ b/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 diff --git a/test/message/nexttick_throw.out b/test/message/nexttick_throw.out index 653eaab7e9a..7f9d4e2babc 100644 --- a/test/message/nexttick_throw.out +++ b/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:*:*) diff --git a/test/message/undefined_reference_in_new_context.out b/test/message/undefined_reference_in_new_context.out index f4e9b5c29ba..983023b4074 100644 --- a/test/message/undefined_reference_in_new_context.out +++ b/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:*) diff --git a/test/simple/test-timers-uncaught-exception.js b/test/simple/test-timers-uncaught-exception.js index 0d2f488df03..6e07fe084e1 100644 --- a/test/simple/test-timers-uncaught-exception.js +++ b/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);