Async - parallel() with functions dynamic array

Question

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?

brandonscript · Accepted Answer · 2014-04-08 03:41:02Z

12

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);
        });
     };
}

edited Apr 8, 2014 at 3:41

brandonscript

73.7k35 gold badges178 silver badges240 bronze badges

answered Sep 18, 2012 at 10:02

ebohlman

15k5 gold badges35 silver badges35 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Fabio B. Over a year ago

I had this suspect! You are really good at explaining it! Thank you 1000 times! :D I'll accept the answer later, let me first try it!

zzen · Accepted Answer · 2012-09-18 22:19:22Z

4

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
    });
};

answered Sep 18, 2012 at 22:19

zzen

1,26910 silver badges13 bronze badges

2 Comments

Fabio B. Over a year ago

Finally I adopted your solution, thank you very much! However this question was just to know how the "closures" mechanism works.

Deniz Ozger Over a year ago

@zzen "results" aren't passed at the end unfortunately

JohnnyHK · Accepted Answer · 2012-09-18 22:36:55Z

2

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        
}

answered Sep 18, 2012 at 22:36

JohnnyHK

313k70 gold badges632 silver badges479 bronze badges

1 Comment

Fabio B. Over a year ago

you're right but I prefer doing single updates this time, just for learning purpose. Thank you.

Collectives™ on Stack Overflow

Async - parallel() with functions dynamic array

3 Answers 3

1 Comment

2 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

2 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related