From 430d94ef85f2786522671317ccbd034ef921ae14 Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 17 Jul 2012 08:17:34 -0700 Subject: [PATCH] nextTick: Preserve depth in error/reentry cases When there is an error that is thrown in a nextTick function, which is then handled by a domain or other process.on('uncaughtException') handler, if the error handler *also* adds a nextTick and triggers multiple MakeCallback events (ie, by doing some I/O), then it would skip over the tickDepth check, resulting in an infinite spin. Solution: Check the tickDepth at the start of the tick processing, and preserve it when we are cleaning up in the error case or exiting early in the re-entry case. In order to make sure that tick callbacks are *eventually* handled, any callback triggered by the underlying spinner in libuv will be processed as if starting from a tick depth of 0. --- src/node.cc | 4 +- src/node.js | 39 +++++++++++--- test/simple/test-domain-stack.js | 4 ++ test/simple/test-next-tick-error-spin.js | 69 ++++++++++++++++++++++++ 4 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 test/simple/test-next-tick-error-spin.js diff --git a/src/node.cc b/src/node.cc index d099840d50..84792d9800 100644 --- a/src/node.cc +++ b/src/node.cc @@ -247,7 +247,9 @@ static void Tick(void) { TryCatch try_catch; - cb->Call(process, 0, NULL); + // Let the tick callback know that this is coming from the spinner + Handle argv[] = { True() }; + cb->Call(process, ARRAY_SIZE(argv), argv); if (try_catch.HasCaught()) { FatalException(try_catch); diff --git a/src/node.js b/src/node.js index b83b890934..466387169b 100644 --- a/src/node.js +++ b/src/node.js @@ -260,8 +260,8 @@ // limit is hit, which is not ideal, but not terrible. process.maxTickDepth = 1000; - function tickDone() { - tickDepth = 0; + function tickDone(tickDepth_) { + tickDepth = tickDepth_ || 0; nextTickQueue.splice(0, nextTickIndex); nextTickIndex = 0; inTick = false; @@ -270,12 +270,35 @@ } } - process._tickCallback = function() { + process._tickCallback = function(fromSpinner) { + + // 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 (inTick) return; inTick = true; // always do this at least once. otherwise if process.maxTickDepth - // is set to some negative value, we'd never process any of them. + // 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; @@ -292,7 +315,9 @@ callback(); threw = false; } finally { - if (threw) tickDone(); + // finally blocks fire before the error hits the top level, + // so we can't clear the tickDepth at this point. + if (threw) tickDone(tickDepth); } if (tock.domain) { tock.domain.exit(); @@ -316,7 +341,9 @@ var tock = { callback: callback }; if (process.domain) tock.domain = process.domain; nextTickQueue.push(tock); - process._needTickCallback(); + if (nextTickQueue.length) { + process._needTickCallback(); + } }; }; diff --git a/test/simple/test-domain-stack.js b/test/simple/test-domain-stack.js index 4d3eeb897a..eecb85b4bb 100644 --- a/test/simple/test-domain-stack.js +++ b/test/simple/test-domain-stack.js @@ -44,3 +44,7 @@ var foo = a.bind(function() { for (var i = 0; i < 1000; i++) { process.nextTick(foo); } + +process.on('exit', function(c) { + if (!c) console.log('ok'); +}); diff --git a/test/simple/test-next-tick-error-spin.js b/test/simple/test-next-tick-error-spin.js new file mode 100644 index 0000000000..4ed31511be --- /dev/null +++ b/test/simple/test-next-tick-error-spin.js @@ -0,0 +1,69 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +var common = require('../common'); +var assert = require('assert'); + +if (process.argv[2] !== 'child') { + var spawn = require('child_process').spawn; + var child = spawn(process.execPath, [__filename, 'child'], { + stdio: 'pipe'//'inherit' + }); + var timer = setTimeout(function() { + throw new Error('child is hung'); + }, 500); + child.on('exit', function(code) { + console.error('ok'); + assert(!code); + clearTimeout(timer); + }); +} else { + + var domain = require('domain'); + var d = domain.create(); + process.maxTickDepth = 10; + + // in the error handler, we trigger several MakeCallback events + d.on('error', function(e) { + console.log('a') + console.log('b') + console.log('c') + console.log('d') + console.log('e') + f(); + }); + + function f() { + process.nextTick(function() { + d.run(function() { + throw(new Error('x')); + }); + }); + } + + f(); + setTimeout(function () { + console.error('broke in!'); + //process.stdout.close(); + //process.stderr.close(); + process.exit(0); + }); +}