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?

Not sure I get it, do you actually have async functions with callbacks, or are you mistakenly using async.eachSeries on a regular array ? — adeneo, Feb 28 '14 at 14:48
I don't really see any reason why callback should be called more than once anyway, it's either called when all done, or once when an error occurs. Following all the nested callbacks is a little hard, but they shouldn't cause the outer callback to execute more than once either, so something strange is happening ? — adeneo, Feb 28 '14 at 15:13

score 4 · Accepted Answer · edited May 23 '17 at 11:43

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.

async.eachSeries callback calling multiple times

1 Answers1