Javascript forEach non blocking query

Question

In my controller (sails.js) I have a array, that contains a list of IDs.

Those are the IDs from a another table. I need to pull each record with that ID, and send it to user.

So far I have made this code:

     suggestions.forEach(function (element, index, array){
                    Suggester.findOne({
                    "id": element.suggester_id
                    },function(err,docs){
                        suggesterResults.push(docs);
                        console.log("I am adding to array: " + docs);
                        if (index === array.length - 1) {
                            completeSend(suggesterResults);
                        }
                    });
                })
    ...
    function completeSend (results) {
        console.log("I am in complete send method"  + results)
        return res.send(results, 200);
    }

That works, but it looks like a cheat. It looks to me this is blocking code, and that is not acceptable. Is there a usual way of doing things in this situation?

Answer 1

The correct way of doing this would be

Suggester.find({
    id: suggestions.map(function(s) { return s.suggester_id; })
})
.then(completeSend);

You call find once, and pass an array of IDs, once that's done, completeSend will be called with the array of results.

---

You mentioned all of this is already being a part of a promise chain, in which case having a .then() there is bad practice (don't nest .then() calls!)

If that's the case, then the correct way would be:

// Some promise chain logic here
.then(function(/* suggestions? */) {
    return Suggester.find({
        id: suggestions.map(function(s) { return s.suggester_id; })
    });
})
.then(completeSend);

Answer 2

With Promises:

Promise.all(suggestions.map(function(suggestion) {
    return Suggester.findOne({"id": suggestion.suggester_id});
})
.then(completeSend);

Lets explain each line:

Promise.all(suggestions.map(function(suggestion) {
    return Suggester.findOne({"id": suggestion.suggester_id});
})

Map the suggestions array into a new array of promises. findOne returns a promise with the future result of the query.

Promise.all() is a static method that takes an array of promises, and returns a single promise that resolves when all of the promises in the array resolve successfully. That promise resolves with an array of all resolved values, in the original order, which happens to be exactly what you want.

.then(completeSend);

The promise this .then is called is the one returned from Promise.all(), so it's the equivalent of calling completeSend() with the array of all docs items, after all of the promises resolved.

Answer 3

If findOne completes asynchronously, and it certainly looks like it does, then that forEach will block for maybe a fraction of a millisecond. Later, findOne will call each of the callbacks, when each of its lookups completes. From a blocking perspective, that code is fine, given the proviso that findOne completes asynchronously.

But, the code has a different issue: You're assuming the callbacks will happen in order, by doing this:

if (index === array.length - 1) {
    completeSend(suggesterResults);
}

You can't make that assumption unless findOne documents it (I looked on the sails.js site; couldn't find any documentation for findOne other than an entry in a bullet list that didn't say anything). The callbacks could arrive out-of-sequence, for instance if one lookup is faster than one before it.

Instead, you'll want to track how many callbacks you've gotten, and call completeSend when you've gotten the same number as the requests you've made, rather than relying on the index.

If suggesterResults is blank when you start and nothing is going to modify suggestions while the calls are outstanding, you can use its length:

suggestions.forEach(function (element, index, array){
    Suggester.findOne({
    "id": element.suggester_id
    },function(err,docs){
        suggesterResults.push(docs);
        console.log("I am adding to array: " + docs);
        if (suggesterResults.length === array.length) {
            completeSend(suggesterResults);
        }
    });
})

But if either of those caveats isn't true, you're better off with a counter:

var waitingon = 0;
suggestions.forEach(function (element, index, array){
    ++waitingon;
    Suggester.findOne({
    "id": element.suggester_id
    },function(err,docs){
        suggesterResults.push(docs);
        console.log("I am adding to array: " + docs);
        if (--waitingon === 0) {
            completeSend(suggesterResults);
        }
    });
})

That looks like a race condition, but it isn't because this is a single-threaded environment. All of the forEach callbacks will happen before the first findOne callback does, so waitingon will rise to the appropriate level before we start decrementing it. (This also allows for the possibilty, which seems unlikely, that suggestions is sparse.)