connection: use ~~process.nextTick~~ setImmediate

[stephenplusplus]

This is really just two types of changes in many places:

1. Use...

   if (err) {
     callback(err);
     return;
   }

   ...as opposed to anything else.

2. Use process.nextTick in two places to simulate an async operation.

[ryanseys]

We should be using setImmediate instead of process.nextTick when executing callbacks. See here for more information.

Pasted here for easy reading:

Callbacks passed to process.nextTick will usually be called at the end of the current flow of execution, and are thus approximately as fast as calling a function synchronously. Left unchecked, this would starve the event loop, preventing any I/O from occurring. setImmediates are queued in the order created, and are popped off the queue once per loop iteration. This is different from process.nextTick which will execute process.maxTickDepth queued callbacks per iteration. setImmediate will yield to the event loop after firing a queued callback to make sure I/O is not being starved.

[stephenplusplus]

From the link:

So in a case where you're trying to break up a long running, CPU-bound job using recursion, you would now want to use setImmediate rather than process.nextTick to queue the next iteration as otherwise any I/O event callbacks wouldn't get the chance to run between iterations.

We're just executing a callback. I'm inclined to stick to nextTick for convention reasons, as well as it's the model scenario for async-ifying sync methods according to the docs: process.nextTick.

[ryanseys]

process.nextTick has the opportunity to starve the event loop and cause a stack overflow when process.maxTickDepth maxes out and I've seen it happen with streaming before due to a misuse of process.nextTick unresolved in node 0.10.x streams.

We should only use process.nextTick in places where setImmediate isn't available. This is even done in the async library. Considering we are only supporting node 0.10.x for this library, we should be using setImmediate everywhere. For all intents and purposes, process.nextTick and setImmediate will do the same thing in this use case, with setImmediate having the added benefit of not shitting the bed if it's called recursively.

[stephenplusplus]

My only reasons for nextTick over setImmediate are based purely on the convention and nextTick documentation. I haven't seen a use of setImmediate to async a sync method, and would feel more comfortable if I did. I do see your points, however I don't think they affect us. It would take a very serious misuse of our library to stack a nextTick queue so high it would choke. Additionally, the async link doesn't do what you say -- it simply uses setImmediate if available in a browser, where there is no nextTick. In this case, setImmediate is much better than setTimeout, 0.

But, yolo. I'll switch it to setImmediate, 'cause why not. It doesn't seem to hurt anything, other than offer a minimal performance loss. Feels weird, but what doesn't in Node?

(@rakyll - feel free to chime in if you prefer it to stick to nextTick)

[ryanseys]

Yeah before either of us make a decision, I would like to hear what @rakyll
thinks. She knows the Node ropes better than I do.

[rakyll]

With node v11.0, immediate queue will be able to tick between process ticks. I think that is going to solve the performance concern mentioned above. Otherwise, LGTM.

See this test: https://github.com/joyent/node/blob/857975d5e7e0d7bf38577db0478d9e5ede79922e/test/simple/test-timers-immediate-queue.js

[ryanseys]

@stephenplusplus You can probably merge this whenever.

[stephenplusplus]

Woo! Thanks for saving us, Ryan. setImmediate for the win!

[ryanseys]

😄 👍