-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Conversation
callback(); | ||
threw = false; | ||
} finally { | ||
if (threw) tickDone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a paranoia case that the following script causes a event loop starvation while one does not in node-v0.8.x.
var domain = require('domain');
var d = domain.create();
d.on('error', function(e) {
console.log(e.message);
f();
});
function f() {
process.nextTick(function() {
d.run(function() {
throw(new Error('d'));
});
});
}
f();
process.on('SIGINT', function() {
console.log('SIGINT');
process.exit();
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's a great catch, thanks!
Fixed on df33211e14d94cb9dee6b717ae93ad845699ce3a
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.
Fixed properly in bb6033a. |
@isaacs Confirmed that it was fixed. |
Landed on master. Thanks for reviewing, everyone. |
@isaacs @bnoordhuis Do you think PrepareTick() and CheckTick() are still necessary? |
Yes, I think they are still necessary. If the tickDepth reaches the maxTickDepth, then we yield to the uv tick spinner, which uses these functions. @piscisaureus @bnoordhuis Is this accurate? |
I do not agree with this tick depth concept. It might be good to print a warning when the loop gets stuck in a nextTick "loop", but breaking out of it seems like sort of random behavior to me that has no merit. It also does not solve any real problems, if this tick depth thing kicks in then nextTick will be "almost" starving the event loop but not completely, which just leaves users confused because their code now suddenly seems very slow and they have to idea why. |
@piscisaureus There could be another way to do it, but we're somewhat caught in a strange position here. On the one hand, we frequently assume that a nextTick will occur before any other I/O. On the other hand, people are using nextTick loops in the wild to perform long-running jobs without blocking pending I/O. Here are the options, as I see them:
I'd actually prefer option 1, and that's what I originally proposed. The feedback was a mix of "Isn't that already how it works?" and "But if you do that, it'll break this program I have." Hence the compromise. 1000 iterations is fast enough to not make too big of a dent on pending I/O, but enough iterations to be way more than anyone will normally hit (if they're not using nextTick recursively forever anyway). A better compromise might be a maximum amount of time that the tick processing loop is allowed to run (say, 1ms or something), but then we'd have to check the time repeatedly, which is significantly costlier than a counter. In order to better serve the non-io-starving-work-queue use case, we should add a process.on('idle') mechanism that will occur whenever there is no pending IO. |
+1
+1
Well,1000 empty iterations are fast enough, sure. But when people are using nextTick to yield then probably they're doing some computation in the callback. All of a sudden this computation will run 1000 times before yielding happens. That's not a nice thing to do. |
@isaacs @piscisaureus @bnoordhuis I wrote down a figure below to show where |
I agree with @shigeki that PrepareTick() and CheckTick() are superfluous now. This snippet, for example, works as you would expect it to: var i = 0;
function tick() {
if (++i < 2000) process.nextTick(tick);
}
process.nextTick(tick); |
bnoordhuis/node@a1a9d21 in case anyone wants to try it. Passes all tests needless to say. |
@bnoordhuis Thanks for your check and approval. I've already tested it and found it works fine. |
@bnoordhuis lgtm. Land it at will. |
Landed in 59b584c. |
Passing all tests on all commits.
@bnoordhuis @piscisaureus