Skip to content
Browse files

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.
  • Loading branch information...
1 parent a52a44e commit 430d94ef85f2786522671317ccbd034ef921ae14 @isaacs isaacs committed
Showing with 109 additions and 7 deletions.
  1. +3 −1 src/node.cc
  2. +33 −6 src/node.js
  3. +4 −0 test/simple/test-domain-stack.js
  4. +69 −0 test/simple/test-next-tick-error-spin.js
View
4 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<Value> argv[] = { True() };
+ cb->Call(process, ARRAY_SIZE(argv), argv);
if (try_catch.HasCaught()) {
FatalException(try_catch);
View
39 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();
+ }
};
};
View
4 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');
+});
View
69 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);
+ });
+}

0 comments on commit 430d94e

Please sign in to comment.
Something went wrong with that request. Please try again.