Node.js For-Loop Inner Asynchronous Callback

Question

I have to following function that assesses if a password is in the user's password history. Here is the code:

isPasswordInPwdHistory : function(redisClient, userId, encPassword, cb) {

        var key = 'user:' + userId + ':pwdhistory';
        redisClient.llen(key, function(err, reply) {
            if (err) {
                status.results = [ ];
                xutils.addStatusResultsItem(status.results, err, null, null, null);
                cb(status);
            }
            else {
                var numPwds = reply;

                var match = false;
                var funcError;

                for (i=0; i <= numPwds - 1; i++) {
                    var error;
                    var pwdValue;
                    redisClient.lindex(key, i, function(err, reply) {
                        if (err) {
                            console.log('lindex err = ' + err);
                            error = err;
                        }
                        else {
                            console.log('lindex reply = ' + reply);
                            console.log('lindex encPassword = ' + encPassword);
                            console.log('lindex (encPassword === reply) = ' + (encPassword === reply));
                            pwdValue = reply;
                        }
                    });

                    console.log('for-loop error = ' + error);
                    console.log('for-loop pwdValue = ' + pwdValue);
                    console.log('for-loop (encPassword === pwdValue) = ' + (encPassword === pwdValue));

                    if (error) {
                        funcError = error;
                        break;
                    }
                    else if (encPassword === pwdValue) {
                        console.log('passwords match');
                        match = true;
                        break;
                    }
                }

                console.log('funcError = ' + funcError);
                console.log('match = ' + match);

                if (funcError) {
                    status.results = [ ];
                    xutils.addStatusResultsItem(status.results, err, null, null, null);
                    cb(status);
                }
                else
                    cb(match);
            }
        });
    }

Here is the console output:

for-loop error = undefined
for-loop pwdValue = undefined
for-loop (encPassword === pwdValue) = false
funcError = undefined
match = false
isPasswordInPwdHistory = false
lindex reply = 5f4f68ed57af9cb064217e7c28124d9b
lindex encPassword = 5f4f68ed57af9cb064217e7c28124d9b
lindex (encPassword === reply) = true

Once I leave the scope of the redisClient.lindex() call I lose the values. How can I pass these values for evaluation in the for-loop?

UPDATE

I refactored the code a bit to handle the matching of the new password (encPassword) vs. the existing password at index i when the redisClient.lindex() callback is issued.

isPasswordInPwdHistory : function(redisClient, userId, encPassword, cb) {

        var status = new Object();
        var key = 'user:' + userId + ':pwdhistory';

        redisClient.llen(key, function(err, reply) {
            if (err) {
                status.results = [ ];
                xutils.addStatusResultsItem(status.results, err, null, null, null);
                cb(status);
            }
            else {
                var numPwds = reply;
                var loopCt = 0;

                for (i=0; i <= numPwds - 1; i++) {
                    loopCt++;
                    redisClient.lindex(key, i, function(err, reply) {
                        if (err) {
                            status.results = [ ];
                            xutils.addStatusResultsItem(status.results, err, null, null, null);
                            cb(status);
                        }
                        else if (encPassword === reply) {
                            status.results = [ ];
                            xutils.addStatusResultsItem(status.results, null, 0, null, true);
                            cb(status);
                        }
                        else if (loopCt === numPwds && encPassword !== reply) {
                            status.results = [ ];
                            xutils.addStatusResultsItem(status.results, null, 0, null, false);
                            cb(status);
                        }
                    });
                }
            }
        });
    }

Unfortunately, caller sees status === undefined even though encPassword === reply is true and I issue the cb(status). How can I exit the for-loop for good after issuing cb(status)?

Michelle Tilley · Accepted Answer · 2012-05-19 18:15:56Z

1

It looks like you have a mix of synchronous and asynchronous thinking in your logic. Your for loop is going to fire off requests to Redis as fast as it can, without waiting for Redis to respond (this is the nature of asynchronous IO). So, by the time your code runs console.log('funcError = ' + funcError); and console.log('match = ' + match);, Redis likely hasn't even responded for the very first value of i in the loop (which matches what you're finding in your output).

I would probably look into a library like async to help with this task; in particular, whilst looks like it might be a good fit. Perhaps something like:

var numPwds = reply;
...
var match = false;
var i = 0;

async.whilst(
  // keep looping until we have a match or we're done iterating the list
  function () { return match == false && i < numPwds; },

  // this is our function to actually check Redis
  function (callback) {
    redisClient.lindex(key, i, function(err, reply) {
      if (err) {
        console.log('lindex err = ' + err);
        error = err;
      }
      else {
        if (encPassword === reply) { match = true; } // this will cause `whilst` to stop.
      }
      i++;
      callback();
    });
  },

  function (err) {
    // this is called when our first function that calls
    // return match == false && i < numPwds
    // returns false, which will happen when we're out of list elements to check
    // or we have a match. At this point, match being true or false
    // let us know if we found the password.
  }
);

As a final note, unless you need to support the same password showing up multiple times in a the "used passwords" list, you can save yourself a lot of heartache by using a set or a sorted set.

answered May 19, 2012 at 18:15

Michelle Tilley

159k40 gold badges379 silver badges312 bronze badges

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

6 Comments

Chris Over a year ago

Brandon, thank you for the feedback! I will definitely checkout async lib. As for using the Redis list datatype: I only worry about storing the last 5 passwords. After a new item is added to this list and it exceeds 5 elements, I ltrim the list back to 5 elements. Truthfully, I don't need the additional memory overhead of a Set/Sorted Set. However, it was very astute of you to point that out. Best to you dude!

Chris Over a year ago

I added a callback under redisClient.lindex().

Michelle Tilley Over a year ago

Chris, the edit (you should probably put the edit on your question) will not work because you are still not handling the found/not found behavior (aka the main callback passed to isPasswordInPwdHistory) in a Redis IO callback--you are handling it in the main path of execution, outside of any Redis callback. There is no way you will be able to make that work--it has nothing to do with losing scope, but rather the order that the code is executed in. You are calling cb(match) before Redis can respond with its results.

Michelle Tilley Over a year ago

Also: the only reason a set would be helpful is you get SISMEMBER, so you can reduce your entire code to a single call to that function. But, if you require keeping things in an order, I can understand the use of a list!

Chris Over a year ago

Updated redisClient.lindex() callback per comments.

|

Collectives™ on Stack Overflow

Node.js For-Loop Inner Asynchronous Callback

1 Answer 1

6 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

6 Comments

Your Answer

Sign up or log in

Post as a guest

Related