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
– J. Ed, Commented Aug 14, 2014 at 17:35
"What am I doing wrong here?" - Using (implicit) global site and options variables. Using for in enumeration on arrays. 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
– Bergi, Commented Aug 14, 2014 at 18:20

Benjamin Gruenbaum · Accepted Answer · 2014-08-14 17:43:18Z

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, 2014 at 17:43

Benjamin Gruenbaum

278k90 gold badges524 silver badges524 bronze badges

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

3 Comments

Surya Over a year ago

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

Benjamin Gruenbaum Over a year ago

@Salman please elaborate - and feel free to ask another question. I have tested the code before posting it and it worked.

Salman Over a year ago

i dont know why cldnt i run may be something on my side so i have deleted my comment sory.

Explosion Pills · Accepted Answer · 2014-08-14 17:39:45Z

0

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

answered Aug 14, 2014 at 17:39

Explosion Pills

192k56 gold badges341 silver badges417 bronze badges

Comments

Leonid Beschastny · Accepted Answer · 2014-08-14 17:39:52Z

0

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).

answered Aug 14, 2014 at 17:39

Leonid Beschastny

51.6k10 gold badges121 silver badges126 bronze badges

2 Comments

Bergi Over a year ago

Actually, using map is a much easier way, so you don't have to manually do promises.push().

Surya Over a year ago

Ah.. closure. why didn't I think of it before.

Collectives™ on Stack Overflow

Node js not resolving array of promises

3 Answers 3

3 Comments

Comments

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

3 Comments

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related