node.js async.js nextTick vs setImmediate

Question

I have a large node.js application that heavily uses the async.js module.

I have a lot of code like this:

async.series([
    function(callback){
        sql.update(query, callback);
    },
    function(callback){
        if (something){
            sql.update(query2, callback);
        }
        else{
            callback(null);
        }
    }

]);

The big problem is the synchronous callback in the else statement. I read a while back that you should not do that with async.js as it could cause unexpected results, but I'm not sure what the best alternative is. I read that I should use process.nextTick in some places, but now I'm reading that we should not use that and it should be setImmediate.

Can someone point me in the right direction? Thanks!

Lythraceous

Can you link to where you read that you shouldn't do that? I've not seen a problem with it. It is the way async should be used. When you are done with the execution of the function you call back. The subsequent function will not run until the callback is called. – Ineptitude

I wish I could find it. I read it a while ago. I know that the callback needs to be called, but I think it's supposed to be wrapped in one of the functions I mention to make the call to the callback asynchronous. We do have a few oddities. Not sure if it's from this or something else, but figured I would remove anything that might be a problem. – Lythraceous

i don't think its the "wrong direction"..the problem should be something else..the first argument of the callback should be null if there is no error otherwise the .series call would just exit with the error. try debugging it with console.trace(), breakpoints and more output. – Staminody

I agree with @Phoenix, I don't think that this is your issue. The fact that the callback is not wrapped in an async call should not make a difference. Either way the callback will be called as a sync operation. The tick that this happens on doesn't matter. I would throw in some logs to see what is really happening. perhaps look at how the callback in the sql operation is being called. – Ineptitude

I should have made clear that there isn't a consistent bug. I know how to fix those :) Every now and then things don't work right. It is not easy to replicate and happens in different places. – Lythraceous

@Lythraceous well your best chance "catching" the cause of this behavior is to log everything write tests that continuously test the script until it produces the error, as your mentioning this isn't a consistent bug but you can at least try to set debug modes on everything and while testing the module you would get all the details if this occurs other than that the aren't many things that nodejs and this script could "tell" , we can't hunt down something which we dont see. – Staminody

if you dont use async.ensureAsync or async.setImmediate (or another async solution like nextTick, setTimeout, etc) you will run into a runtime exception as the memory will grow and grow. This is because a synchronous callback will let the heap grow and the garbage collector cant run – Scabrous

Answer 1

I FINALLY found the reference I remember reading a while back.

It is on this page: http://caolanmcmahon.com/posts/nodejs_style_and_structure/

He discusses the general case in rule #3 and in more direct correlation to my issue in response to the first comment on the page.

Even if it's not the cause of my random error, I would like to use the module according to the author's intentions which was the point of the question.

In this article, he mentions using process.nextTick, but I really think node is trying to move away from that in this case and I found a response in a async issue thread from a month ago that says:

"You need to make sure your functions are running asynchronously - if you don't know whether it'll be asynchronous or not you can force it to be async using setImmediate"

So, that's the answer I'm going with. I'm going to wrap any synch callbacks in setImmediate.

Lythraceous