Node js not resolving array of promises

Question

I'm trying to execute several async requests and trying to get the output using promises.

If I've multiple requests queued up the Q.all(promises).then () function doesn't seem to be working. For a single request the promises are all resolved. The sample code is here.

var request = require('request');
var Q = require('q');

var sites = ['http://www.google.com', 'http://www.example.com', 'http://www.yahoo.com'];
// var sites = ['http://www.google.com']

var promises = [];

for (site in sites) {
  var deferred = Q.defer();
  promises.push(deferred.promise);
  options = {url: sites[site]};

  request(options, function (error, msg, body) {
    if (error) {
      deferred.reject();
    }

    deferred.resolve();
  });
}

Q.all(promises).then (function () {
  console.log('All Done');
});

What am I doing wrong here ?

Surya

you'll need to provide more details- what exactly happens when you run the above code? — J. Ed, Aug 14 '14 at 17:35
"*What am I doing wrong here?*" - Using (implicit) global `site` and `options` variables. Using [`for in` enumeration on arrays](http://stackoverflow.com/q/500504/1048572). Also, the reason why it fails: your `deferred` will be overwritten by the subsequent calls, your callback is not a closure over it - it always tries to resolve the single, last `deferred`. — Bergi, Aug 14 '14 at 18:20

score 3 · Accepted Answer · answered Aug 14 '14 at 17:43

3

Here is what I'd do in the scenario, this is the whole code:

var Q = require('q');
var request = Q.nfbind(require('request'));

var sites = ['http://www.google.com', 'http://www.example.com', 'http://www.yahoo.com'];
var requests = sites.map(request); 

Q.all(requests).then(function(results){
   console.log("All done") // you can access the results of the requests here
});

Now for the why:

Always promisify at the lowest level possible, promisify the request itself and not the speicific requests. Prefer automatic promisification to manual promisification to not make silly errors.
When working on collections - using .map is easier than manually iterating them since we're producing exactly one action per URL here.

This solution is also short and requires minimal nesting.

answered Aug 14 '14 at 17:43

Benjamin Gruenbaum

270,886
87
504
504

I realised my mistake to be one with closures. But I'm going to accept this answer as I found out another way of dealing with promises. Thanks – Surya Aug 14 '14 at 18:22
@Salman please elaborate - and feel free to ask another question. I have tested the code before posting it and it worked. – Benjamin Gruenbaum Feb 21 '15 at 17:29
i dont know why cldnt i run may be something on my side so i have deleted my comment sory. – Salman Feb 21 '15 at 17:38

score 0 · Answer 2 · answered Aug 14 '14 at 17:39

Don't use for..in to iterate over arrays. What this actually does is set site to 0, 1, 2 and this just doesn't work very well. Use some other form of iteration like a regular for loop, or Array.prototype.forEach

sites.forEach(function (site) {                                                 
  var deferred = Q.defer();                                                     
  promises.push(deferred.promise);                                               
  options = {url: site};

score 0 · Answer 3 · answered Aug 14 '14 at 17:39

The problem is that you're changing the value of deferred on every tick of your for loop. So, the only promise which is actually resolved in your example is the last one.

To fix it you should store the value of deferred in some context. The easiest way to do so is to use Array.prototype.forEach() method instead of for loop:

sites.forEach(function (site){
  var deferred = Q.defer();
  var options = {url: sites[site]};
  promises.push(deferred.promise);

  request(options, function (error, msg, body) {
    if (error) {
      deferred.reject();
    }

    deferred.resolve();
  });
})

And you also missed var declaring options variable. In JavaScript it means declaration of a global variable (or module-wide variable in node.js).

Actually, using `map` is a much easier way, so you don't have to manually do `promises.push()`. — Bergi, Aug 14 '14 at 18:22

Node js not resolving array of promises

3 Answers3