-
Notifications
You must be signed in to change notification settings - Fork 7.3k
nextTick documentation and warning message are confusing #6718
Comments
Since we can't swap the names of |
nextTick and setImmediate are one of the things of node.js (along with the streams implementation) that cause more confusion and are harder to learn, basically because the documentation leads to a bad understanding of the problem. |
setImmediate runs once per event iteration. nextTick runs multiple times per iteration. That's all the difference there is. If your code depends on what fires first in series of deeply nested callbacks, you're doing it wrong. |
@rlidwka amen. @davepacheco if you want to draft the changes that would be great, otherwise in the short term I'll land a rewording of the error message in master. |
and by master, I meant v0.10 since the warning doesn't exist in master |
@othiym23 and others: Thanks. I submitted a PR with suggested changes for the docs. |
@sam-github , oh wow, I didn't know that they landed it. Well, now there is almost no difference between them at all. Time to deprecate something. :) |
I am still of the opinion that fix should have gone back to v0.10 but neither here nor there, it was a deficiency in my initial implementation that was not caught. If you could have been with me at lunch you would have heard me going on at length about how excited I was to deprecate I did a quick test swapping I haven't done any benchmarking -- though @trevnorris suggests there may be some impact, I doubt any impact is insurmountable. The path forward as I see it,
|
I'm not sure actually deprecating nextTick is very friendly, it was the recommended way to some things for a long time, how about making nextTick and setImmediate identical? |
If you want them to be identical, they should exist in the same object and have the same name. Let's move nextTick from process to global and rename it to setImmediate, I'm fine with that. :) |
This decision making process is actually particularly hard: On the one hand I am relatively in favor of changing the implementation to be However that seems particularly impolite to swap out the implementation under the user without actually notifying them. It seems like it's most polite to leave the implementation there, deprecate it, and mark it as receiving no further updates. There is a semantic change happening here, and while I'm not fond of |
Wait, what? I was under the impression that
If If |
Performance. Allowing a consistent async API, but still running the async callback before other I/O has had a chance to process. And, um, performance :) I'm sure @tjfontaine and I are going to have a fun discussion about the deprecation of Contrived example, but think the concept is applicable. Say we want to be able to queue up a bunch of I/O work to be done in the following way. function runner(arg, cb) {
var err = checkArg(arg);
if (err)
setImmediate(cb, err);
continueNormally(arg, cb);
} Problem is now if we run function runner(arg, cb) {
var err = checkArg(arg);
if (err)
// Yes, I know the anon function semantics aren't optimal, but for
// the sake of the example.
process.nextTick(function() { cb(err); });
continueNormally(arg, cb);
} Now, from the callback we can do the following easily: function cb(err) {
if (err)
cancelRunners();
} So each request can be flagged to not go out before the transaction takes place. For all those who want them to be identical, here's the complete patch for you to try out: https://gist.github.com/trevnorris/8011423 |
@trevnorris ... any reason to keep this one open? |
Maybe we can switch nextTick and setImmediate? Seriously, we should think about it for 3.x. And I have not yet managed to find any coherent explanation from anyone (much less our docs) under what conditions one should choose one or the other. @trevnorris your example makes interesting reading, but what does it have to do with performance? make all your calls to checkArgs(), then if they all succeed, make all the calls to continueNormally()... it might be fun to use the nextTick queue like that, but it sure doesn't look necessary. Can you point to a place in node where swapping nextTick for setImmediate would cause a performance regression? Right now, my best summary of when to use one vs the other would be:
This isn't great for people writing code that needs portability across a range of node versions. Anyhow, they are fundamental APIs, and it should be possible to make some kind of brief and sensible statement about them, perhaps: "always use setImmediate, nextTick is legacy but still kept around for backwards compatbility, because it is used by some for its unique queueing semantics, and because years ago it used to be faster". |
@sam-github My example isn't about performance. It's about usability. Specifically when you want the callback to be asynchronous but want it to be called before the event loop proceeds. For example: var server = require('net').createServer().listen(8080, listening);
server.on('connection', function onConnection(c) {
// ...
});
var foo = 'bar';
function listening() {
console.log(foo);
} Internally, binding and listening to a port is synchronous on at least Linux. Though to maintain the "asynchronous" nature we still allow a callback to be called. If the |
I think you need to rewrite your example as:
But point taken, we expect onListening before onConnection. If I understand, you are saying a sequence such as the following is possible: . sync socket()/bind()/listen() because if that was so, setImmediate could work if the wrap of the sock as a connection and emit of such as an event was itself delayed by a setImmediate. I take your point about nextTick always going to be faster under some conditions. If you are doing nextTicks one after the other, they will occur synchronously, with no interleaving epoll's, so will be "faster". This comes at a price, however. Taking your example further, with the now infinite nextTick depth, the following code, while appearing async, is actually a synchronous busy-loop in the next tick handler:
nextTick and setImmediate obviously do slightly different things, with slightly different behaviours, I'm not totally convinced ATM that having both of them at the same time, though, is better than having only one of them. One issue is that nextTick and setImmediate both have determinate ordering - they queue in order of call, guaranteed. Very useful. But if you intermix the two, you have no such guarantees. And if you use nextTick, you don't have any particular guarantee that node will EVER return to the event loop. I've debugged subtle problems with this, where nextTick was used, as in the sync listen, to create "seeming asynchrony", but since the epoll loop never actually "ticks over", if your caller assumes that its getting real asynchrony, that deriving from epoll, you can actually get a busy loop. I don't really want to convince you that nextTick should be banished from node (though I have yet to convinced that it shouldn't be at least discouraged), but I do think a clear statement about when to use the two should be in the docs, not just subtle examples of how to blow your foot off with them in github conversations. |
@sam-github The two simplest places I think |
A user of node-vasync reported this warning message emitted by Node on the latest v0.10:
This is a classic misuse of process.nextTick: the caller is using a vasync Queue with a synchronous worker function, and the Queue dispatches each new task using process.nextTick. As a result, once the first task completes, all subsequent ones are dispatched and completed on the same tick. If that number is more than 1000, we get this warning (though the program is arguably operating correctly, as this is not an infinite loop).
While this is sort of a programmer error, I think there are two concrete Node changes that would improve the situation.
First is the warning message. There's no recursion going on here. It's just a loop. The code refers to "depth", but it's not stack depth -- it's just a loop counter (as the code comment says). I get the sense in which it feels recursive, in that the function is triggering a call back into itself, but it's confusing to say that recursion is the problem because the real problem (which is only potentially detected) is event loop starvation, and that's not normally a result of recursion. I think this would be a clearer error message:
That makes it much clearer what actually happened, what Node did about it, why that's still a problem for the developer, and what to do about it.
The second improvement is for the documentation for process.nextTick, which I think leads people down the wrong path. There appear to be two common use cases for nextTick():
I take as given that case 1 is useful, but case 2 is toxic because in case 2 there's no way to be sure to avoid the recursion problem. Users must always use setImmediate() for case 2. Since I think case 2 is overwhelmingly more common than case 1, I think the docs should strongly downplay any use of process.nextTick in favor of setImmediate. Specifically:
I'd be happy to draft these changes, if this sounds good.
The text was updated successfully, but these errors were encountered: