async.eachSeries callback calling multiple times

Question

In this function:

function method2(friends, callback) {
    //friends is an array of objects
    var ids = _.pluck(friends, 'id'),
        arrays = cut(ids, 24),
        //cut() splits array into smaller arrays of given length 
        code = require('fs').readFileSync('...').toString();
    var imp,j;
    async.eachSeries(arrays, function(i, cb1) {
        ...
        vk.request('execute', {code:code}, function(err, resp, body) {
            //vk.request passes its callback to node-request module
            //at this point, err is null, and body.error is undefined
            if(err || body.error) return cb1(err || body.error);
            var arr = body.response;
            for(var e in arr) {
                if(!arr[e]) return cb1();
                async.eachSeries(arr[e], function(i, cb) {
                    ...
                    cb();
                }, cb1);
            }
        })
    }, callback);
}

function is called only once, but async calls callback many times without providing any arguments to it. I cant't see any reasons why. so what's wrong with this code?

Answer 1

I think your problem is here:

for(var e in arr) {
    // ...
    async.eachSeries(/* ... */, cb1);

You are calling cb1 multiple times, which causes the outermost async.eachSeries to continue multiple times, and therefore the final callback to be called multiple times.

Solution: use async.each instead of a simple for loop to spawn multiple concurrent inner async.eachSeries loops (if that's really what you want). This is the way to nest async loops inline:

async.eachSeries(/* ... */, function(/* ... */, cb1) {
  // this body runs once at a time
  async.each(/* ... */, function(/* ... */, cb2) {
    // this body runs multiple times 'concurrently'
    async.eachSeries(/* ... */, function(/* ... */, cb3) {
       // this body runs sequentially,
       // but multiple sequential runs can happen at once
       cb3(/* ... */);
    }, cb2);
  }, cb1);
}, callback);

An off-topic bonus: Using readFileSync is not advisable except at application startup (if and only if it's safe to use require, it's also safe to use readFileSync). Since you're using async calls, I must assume this is a transactional function, so you should change that to fs.readFile with a callback.

Second bonus: Of course, taken too far, this kind of nesting turns into a big mess. There are ways to combat this using functional programming techniques.