CODEWITHSUNDEEP
node.jsasynchronousredis
pchmn
pchmn

Reputation: 534

For loop in redis with nodejs asynchronous requests

I've got a problem with redis and nodejs. I have to loop through a list of phone numbers, and check if this number is present in my redis database. Here is my code : 

function getContactList(contacts, callback) {
  var contactList = {};

  for(var i = 0; i < contacts.length; i++) {
    var phoneNumber = contacts[i];

    if(utils.isValidNumber(phoneNumber)) {

      db.client().get(phoneNumber).then(function(reply) {
        console.log("before");
        contactList[phoneNumber] = reply;
      });
    }
  }

  console.log("after");
  callback(contactList);

};

The "after" console log appears before the "before" console log, and the callback always return an empty contactList. This is because requests to redis are asynchronous if I understood well. But the thing is I don't know how to make it works. How can I do ?

Upvotes: 1

Views: 1093

Answers (4)

jfriend00
jfriend00

Reputation: 707466

You have two main issues.

  1. Your phoneNumber variable will not be what you want it to be. That can be fixed by changing to a .forEach() or .map() iteration of your array because that will create a local function scope for the current variable.

  2. You have create a way to know when all the async operations are done. There are lots of duplicate questions/answers that show how to do that. You probably want to use Promise.all().

I'd suggest this solution that leverages the promises you already have:

function getContactList(contacts) {
    var contactList = {};
    return Promise.all(contacts.filter(utils.isValidNumber).map(function(phoneNumber) {
        return db.client().get(phoneNumber).then(function(reply) {
            // build custom object
            constactList[phoneNumber] = reply;
        });
    })).then(function() {
        // make contactList be the resolve value
        return contactList;
    });
}

getContactList.then(function(contactList) {
    // use the contactList here
}, funtion(err) {
    // process errors here
});

Here's how this works:

  1. Call contacts.filter(utils.isValidNumber) to filter the array to only valid numbers.
  2. Call .map() to iterate through that filtered array
  3. return db.client().get(phoneNumber) from the .map() callback to create an array of promises.
  4. After getting the data for the phone number, add that data to your custom contactList object (this is essentially a side effect of the .map() loop.
  5. Use Promise.all() on the returned array of promises to know when they are all done.
  6. Make the contactList object we built up be the resolve value of the returned promise.
  7. Then, to call it just use the returned promise with .then() to get the final result. No need to add a callback argument when you already have a promise that you can just return.

Upvotes: 4

paulsm4
paulsm4

Reputation: 121689

Consider refactoring your NodeJS code to use Promises.

Bluebird is an excellent choice: http://bluebirdjs.com/docs/working-with-callbacks.html

Upvotes: 0

Jason Livesay
Jason Livesay

Reputation: 6377

The simplest solution may be to use MGET with a list of phone numbers and put the callback in the 'then' section.

You could also put the promises in an array and use Promise.all().

At some point you might want your function to return a promise rather than with callback, just to stay consistent.

Upvotes: 1

boehm_s
boehm_s

Reputation: 5544

you put async code into a for loop (sync operations). So, each iteration of the for loop is not waiting for the db.client(...) function to end.

Take a look at this stackoverflow answer, it explains how to make async loops :

Here

Upvotes: 0

Related Questions