Async - parallel() with functions dynamic array

Question

Async - parallel() with functions dynamic array

I am using the excellent caolan "async" module for nodejs:

I have this code:

exports.manageComments = function(req, res) {
    var toDeleteIds = [];
    var deleteFunctions = [];
    if (req.body.doDelete) {
        toDeleteIds = req.body.doDelete;
    }
    var i;
    for ( i = 0; i < toDeleteIds.length; i++ ) {
        var deleteFunction = function(callback) {
            var id = toDeleteIds[i];
            console.log(id);
            Comment.findOne({_id:id}, function(err, found) {            
                if (!err) found.remove(callback);
            });
        }
        deleteFunctions.push(deleteFunction);
    }
    async.parallel(
        deleteFunctions,
        function(err,results) {
            exports.comments(req, res); //render a view
        }
    );
};

My array contains two elements but console.log() keeps telling me "undefined".

What am I missing?

javascript
node.js
express
mongoose
async.js

Answer 1

Your problem is with:

    var deleteFunction = function(callback) {
        var id = toDeleteIds[i];

because at the time each callback function is executed, i will have the same value as toDeleteIds.length. A closure doesn't "trap" the value that an outer variable had at the time it was created; it "traps" a reference to whatever value the outer variable has at the time it's executed (which in this case won't be until well after your for loop has finished.

In order to "trap" the value of i at the time you create your callback function, you need to make i a parameter of a function that you call to create your callback function. You need something like

    var deleteFunction = makeDeleteFunction(i, callback);

And then create a separate function outside the callback:

function makeDeleteFunction(i, callback) {
    return function(callback) {
        var id = toDeleteIds[i];
        console.log(id);
        Comment.findOne({_id:id}, function(err, found){            
            if (!err) found.remove(callback);
        });
     };
}

Answer 2

ebohlman has correctly identified the problem. However I still think creating an array of closures is hugely inefficient and unnecessary. Here's shorter, easier code to achieve the same with a single function:

exports.manageComments = function(req, res) {
    var toDeleteIds = [];
    if (req.body.doDelete) {
        toDeleteIds = req.body.doDelete;
    }

    var deleteFunction = function(id, callback) {
        console.log(id);
        Comment.findOne({_id:id}, function(err, found) {            
            if (!err) found.remove(callback);
        });
    }

    async.forEach(toDeleteIds, deleteFunction, function(err,results) {
        exports.comments(req, res); //render a view
    });
};

Answer 3

Looking at this another way, if you don't need to fire the Mongoose remove middleware on each Comment doc being removed, you can remove all identified comments in one go:

Comment.remove({_id: {$in: toDeleteIds}}, function(err, numRemoved) {
    exports.comments(req, res); //render a view        
}