2

I am trying to run some database queries (using sails.js) on an array and upon the queries' return, do something. I figured the best way to do so would be to use a for loop and resolve the promises async, and once they've all resolved, continue on. However, only the last promise in my array is resolving, and it is resolving multiple times because in each 'User.findOne...' then function, the index is array.length-1.

My questions:

  1. How does variable scope in asynchronous loops work? Best resources to explain this?
  2. What is the best way to solve my problem? Why?
  3. Are there any other patterns I should use or not use? I am fairly new to promises and async js, so any tips would be helpful!

Main tutorials I've checked

Thank you for your help!


My simplified code:

functionWhichReturnsPromise()
.then(function(user){
  var promises = [];
  Q.try(function(){
    for (var index in array) {
      var fbid = array[index];// Get fbid from array
      promises.push(Q.defer().promise); // Add promise to promise array

      // Find userid from fbid; resolve respective promise when finished
      User.findOne({facebook_id: fbid}).then(function(userSeen){
        promises[index].resolve(userSeen.id);
        sails.log('resolved where id=' + userSeen.id); // correct
        sails.log('resolved where index=' + index); // PROBLEM: always last index
      });
    }
  }).then(function(){

    // For debugging purposes
    Q.delay(1000).then(function(){
      sails.log(promises[0]); // Unresolved
      sails.log(promises[1]); // Unresolved
      sails.log(promises[2]); // Only last promise in array is resolved
    });

    // When the userids have been extracted from above (promises fulfilled)...
    Q.all(promises).then(function(seenids){
      // Do stuff here (Doesn't get here)
    });
  });
});
Travis Webb
  • 14,688
  • 7
  • 55
  • 109
smileham
  • 1,430
  • 2
  • 16
  • 28
  • 1
    Yes, you are using the [deferred antipattern](http://stackoverflow.com/q/23803743/1048572). Also, there's not really a reason to use `Q.try` in here. – Bergi Jul 03 '15 at 08:53
  • @Bergi, I read your reference on the deferred antipattern, but I am not quite sure how I would implement this without deferred objects. I have n parallel processes to execute, and when they are finished I need to use their data. Using .then wouldn't work on a single promise since I need to wait until they all finish. How would I go about implementing that? Also would I just use Q()? – smileham Jul 05 '15 at 05:45
  • 1
    You are not using the deferreds to await them for finishing - you use `Q.all` (as you should). The deferreds are only used to construct each of those `promises`, and they aren't needed for that. You should be doing just `promises.push(Q(User.findOne(…)).then(function(userSeen) { …; return userSeen.id; }))`. You probably can even omit the `Q()` call, as `findOne()` already returns a promise – Bergi Jul 05 '15 at 10:30
  • @Bergi - Thank you. I think I understand what you mean, but I couldn't get a simpler version to work, and I think I'm missing something. I posted a new question as I was realizing how related it was to this one, so maybe you could show me what you mean; I'd be more than happy to accept your answer. http://stackoverflow.com/questions/31551638/node-js-sails-js-function-in-foreach-loop – smileham Jul 22 '15 at 00:06

1 Answers1

2

In Javascript, variable's scope is function and not curly braces.

Therefore in the following code, the scope of var index is not the for loop's curly braces, the scope is actually the function in which the for loop exists.

Q.try(function(){
    for (var index in array) {
      var fbid = array[index];// Get fbid from array
      promises.push(Q.defer().promise); // Add promise to promise array

      // Find userid from fbid; resolve respective promise when finished
      User.findOne({facebook_id: fbid}).then(function(userSeen){
        promises[index].resolve(userSeen.id);
        sails.log('resolved where id=' + userSeen.id); // correct
        sails.log('resolved where index=' + index); // PROBLEM: always last index
      });
    }
  })

Within for loop you call the async function, in your case its mongodb call (findOne). You should always assume that these async function can take any number of milliseconds to run (depends on the function). But in general, usually the loop would have completed before the async functions run. Your for loop fires all those async functions even before those functions start running. The issue is that all those async functions which area pending are still pointing towards that variable index. And that variable is common to all of them because index was in scope of the outer function.

This is a problem created somewhat because of closures in Javascript. And to solve this, we need to use more closures.

There are many resources on the topic of closures that you can google. But go thru the MDN's description of it.

If you capture value of index within another function inside the loop, then you will be good to go.

Here is my suggested solution to your issue. I haven't tested it though, but you get the idea.

Q.try (function () {
    array.forEach( function(ele, idx, array) {
        (function(index) {
            var fbid = array[index]; // Get fbid from array
            promises.push(Q.defer().promise); // Add promise to promise array

            // Find userid from fbid; resolve respective promise when finished
            User.findOne({
                facebook_id : fbid
            }).then(function (userSeen) {
                promises[index].resolve(userSeen.id);
                sails.log('resolved where id=' + userSeen.id); // correct
                sails.log('resolved where index=' + index); // PROBLEM: always last index
            });
        })(idx);
    })
})

Hope this helps.

Also Note: it is incorrect to use for...in for iterating through arrays.

Community
  • 1
  • 1
bits
  • 8,110
  • 8
  • 46
  • 55
  • Thank you! I was able to solve it with your help, and I'll definitely look into closures. Ah yeah, I forgot about for...in; thanks for reminding me. – smileham Jul 03 '15 at 01:10