Async function and callbacks inside a forEach loop

Question

Inside this function:

function Example(array, callback){
    var toReturn = [];

    // variable 'array' has 2 elements in this case
    array.forEach(function(el){
        MongooseModel.AsyncFunction(el, function(err, doc){
            if(err || !doc){ return callback('Error'); }
            toReturn.push(doc)
        });
    });

    return callback(null, toReturn)
}

A few things to note:

1. toReturn.push(doc) doesn't work and on the last callback an empty array is returned. Why is that?
2. If there's an error, or !docs is true, return callback('Error') is never called. return callback(null, toReturn) is always called.

Also, if I put a callback above the async function, like this:

function Example(array, callback){
    var toReturn = [];

    // variable 'array' has 2 elements in this case
    array.forEach(function(el){
        // another callback here
        return callback('Error')
        MongooseModel.AsyncFunction(el, function(err, doc){
            if(err || !doc){ return callback('Error'); }
            toReturn.push(doc)
        });
    });

    return callback(null, toReturn)
}

the script crashes because multiple callbacks have been called. Why is forEach doing that and how to avoid it?

Answer 1

Your questions

toReturn.push(doc) doesn't work and on the last callback an empty array is returned. Why is that?

You're calling callback before MongooseModel.AsyncFunction gets any chance to execute your function(err,doc).

If there's an error, or !docs is true, return callback('Error') is never called. return callback(null, toReturn) is always called.

The successful return is called immediately after scheduling your async functions. It's still possible that the callback('Error') will be called sometime later by MongooseModel.AsyncFunction.

the script crashes because multiple callbacks have been called

That's exactly the behaviour you asked for! Your second code literally says: "For each element, call the callback once".

---

Some pointers

All in all, I think the single thing you have wrong is that calling the callback somewhat resembles returning from the function.

That's not the case! Your function Example is supposed to schedule some async things to happen, then return immediately (returning nothing) but promising that either callback(error) or callback(null, success) will be called sometime later in the future.

Therefore, it doesn't make sense to ever say return callback() - just callback() will do. By the time that you have some data to call callback with, the function Example will already have finished executing. The function that eventually calls callback will the an anonymous function passed to MongooseModel.AsyncFunction, not Example itself.

---

Now what?

You could try an approach like this:

function Example(array, callback){
    var toReturn = [];
    var previousError = false;

    array.forEach(function(el){
        MongooseModel.AsyncFunction(el, function(err, doc){
            if (previousError) {
                return;
            }
            if(err || !doc) {
                previousError = true;
                callback('Error');
            }
            toReturn.push(doc)
            if (toReturn.length === array.length) {
                // that was the last push - we have completed
                callback(null, toReturn);
            }
        });
    });
}

Stuff that happens here:

• Every time your AsyncFunction completes, you aggregate the result. If that was the last one, you can finally call callback and pass the whole data. (Otherwise, we're waiting for a few more functions, so no callback)

• If there was an error somewhere along the way, we report it immediately, but also take a note that the error has been reported already, so that further executions of AsyncFunction that have been scheduled don't do anything in particular. This prevents your callback from potentially being called twice or more.

• Be careful though - the order of elements inside toReturn will be random, depending on which async tasks complete first.

---

But this is messy

Oh yes. That's why we don't really do callbacks anymore. There's a pattern called promises that makes it significantly easier to work with async callback spaghetti, I suggest to read up a little on this topic before moving forward.

• https://www.promisejs.org/

Let's make it neat

Using promises, this kind of code would look something like:

function Example(array) {
    var promises = array.map(function(el) {
        return MongooseModel.AsyncFunction(el);
    });
    return Promise.all(promises);
}

which is much shorter and also doesn't have any problems with error handling or ordering of output items like our previous example.

Usage is simple - instead of this

Example([1, 2, 3], function(error, results) {
    if (error !== null) {
        // ...
    } else { 
        // ...
    }
})

you call like this:

Example([1, 2, 3]).then(
    function(results) {
        // ...
    },
    function(error) {
        // ...
    }
);

Oh, this relies on MongooseModel.AsyncFunction returning a promise on itself, but Mongoose already does this and so do most libraries. If a library doesn't, you can easily add the promise support (see NodeRedis for example).

How to do something with each result of AsyncFunction before returning it?

Easy too!

function Example(array) {
    var promises = array.map(function(el) {
        return MongooseModel.AsyncFunction(el).then(function(doc) {
            return doc + "some modifications";
        });
    });
    return Promise.all(promises);
}

Or if you need additional error handling:

function Example(array) {
    var promises = array.map(function(el) {
        return MongooseModel.AsyncFunction(el).then(function(doc) {
            if (theresSomethingWrongWith(doc) {
                return Promise.reject(new Error("ouch!"));
            }
            return doc + "some modifications";
        });
    });
    return Promise.all(promises);
}

Think of returning a rejected promise as something similar to raising an exception - it will naturally bubble up all the way to the returning promise of Example.

Answer 2

Basically, your function executes in the following order:

1. Trigger the async action for each element in array
2. Return immediately after all the async calls were triggered
3. Async calls return with results, but your function returned already - so they are ending up nowhere.

In order to perform multiple async calls and return their results, you need to wait for all of them to finish. I usually solve this problem with the help of the library async, which even gives you control over how the async calls should be executed (in series or in parallel). For example, you can call async.parallel with an array of async functions:

async.parallel(array.map(function() {
  return MongooseModel.AsyncFunction(...)
}), function(err, results) {
  // all the async calls are finished, do something with the results
});

Answer 3

You should use promises because you need to wait when all async calls will be done before returning result. Here is example:

function Example(array, callback){
    new Promise(function(resolve, reject) {
        var toReturn = [];

        array.forEach(function(el){
            MongooseModel.AsyncFunction(el, function(err, doc){
                if(err || !doc) {
                    return reject();
                }
                toReturn.push(doc);
                if (toReturn.length === array.length) {
                    resolve(toReturn);
                }
            });
        });
    })
        .then(callback)
        .catch(function() {
            callback('error');
        });
}

And here you can read more about promises: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Promise