Redis, node.js and Javascript Callback Scoping / Flow

Question

I've been exploring javascript more deeply lately, playing around with a node,redis,socket.io,express simple webapp.

I think I've come to a problem that is an example of the fundamental difference between JS and PHP, which is what I've been using the last few years.

I've written this callback and need to return a stack of data from redis

app.get('/deck', function(request, response) { 
  dealer.sort('cards', 'BY', 'deal:*->timestamp', 'GET', 'card:*->card_key',
    function(err, card_keys) {
      var deck = []; 
      for (k in card_keys) { 
        dealer.hget(card_keys[k], 'data', function(err, card_data) { 
          var deal = eval('(' + card_data +')');
          deals.push(cards);
        }); 
      }   
      response.json(deals);
    }   
  );  
});

Now first I thought it was a variable scoping problem so I rewrote with a closure (did I use this term correctly?) which didn't work because I am writing this in synchronous mind set. I realize that this is essentially synchronous and sends the data out before it is collected. The design is wrong.

I can rewrite the function to use socket.io's emit function to send data as it is collected.

But are there design patterns for handling synchronous data? What if I want to show the last 10 cards in order? Do I send the data over anyway and have client side code queue up 10, and trigger an event that sorts and then control display?

Edit: I found this question which essentially mine, except that I would like to figure out how to design this without relying an synchronous library.

you can't use async function in your for loop. You should use something like async or streamlinejs. — malletjo
– malletjo, Commented Nov 24, 2011 at 0:53
Thanks, but for my own good, I would like to understand how to structure this in a callback way without relying on those libraries. — jon skulski
– jon skulski, Commented Nov 24, 2011 at 1:18

Raynos · Accepted Answer · 2011-11-24 01:20:21Z

4

function(err, card_keys) {
    var deck = [];
    for (k in card_keys) {
        // whole bunch of async actions
        dealer.hget(card_keys[k], 'data', function(err, card_data) {
            var deal = eval('(' + card_data + ')');
            deals.push(cards);
        });
    }
    // sync action
    response.json(deals);
}

You already know response.json fires before the hget are done.

The answer is reference counting

function(err, card_keys) {
    var deck = [];
    var count = Object.keys(card_keys).length;

    function next() {
        if (--count === 0) {
            response.json(deals.sort(sortThem)); 
        }  
    }

    for (k in card_keys) {
        dealer.hget(card_keys[k], 'data', function(err, card_data) {
            var deal = JSON.parse(card_data);
            deals.push(cards);
            next();
        });
    }
}

You have two other minor problems

using eval instead of JSON.parse
your deals array is unordered because hget is asynchronous, so you need a sorting function.

answered Nov 24, 2011 at 1:20

Raynos

170k57 gold badges359 silver badges399 bronze badges

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

3 Comments

jon skulski Over a year ago

Thanks for the answer, trying to grok this all. One quick question, why Object.keys(card_keys).length instead of card_keys.length?

Raynos Over a year ago

@jskulski wasn't sure if card_keys was an array. I mean you did use a for..in loop over it and for..in over an array is a bad practice

jon skulski Over a year ago

@Raynos well I'm very new at this :) Thanks for all the tips, appreciate it.

malletjo · Accepted Answer · 2011-11-24 04:01:48Z

2

One way to achieve this without using another library is using the "recursive pattern".

You can't do some asynchronious call in a for loop.

card_keys need to be an array.

Some useful link about how to write for loop :

EDIT: Like Raynos said, you should use reference counting instead.

I suggest you use a lib and do something like this with async:

...
var deals = []
async.forEach(card_keys, function(err, results) {
    if (err) {
        callback(err);
    } else {
        deals.push(results); // For each card
    }
}, function(err) {
    callback(deals); // When it's completed
});
...

This is how async is doing the forEach

async.forEach = function (arr, iterator, callback) {
    if (!arr.length) {
        return callback();
    }
    var completed = 0;
    _forEach(arr, function (x) {
        iterator(x, function (err) {
            if (err) {
                callback(err);
                callback = function () {};
            }
            else {
                completed += 1;
                if (completed === arr.length) {
                    callback();
                }
            }
        });
    });
};

Edit: Which one is faster : reference counting or recursive pattern

I did 100 basic query :

Resurcive pattern : 7161,7528, 7226 MS

Reference counting: 7515, 7256, 7364 MS

Recursive pattern code :

var req = "select id from membre";

var time = new Date().getTime();

var test = function(value,callback) {
   if(value < 100) {
      client.query(req, function(err, results) {
          test(++value,callback);
      });
   } else {
       callback();
   }
}

test(0, function() {
   var time2 = new Date().getTime();
   console.log(time2-time);
   client.end();
});

Reference counting code :

var req = "select id from membre";

var time = new Date().getTime();

var test = function(callback) {

   var c = 100;
   function next() {
      if(--c === 0) callback();
   }

  for(var i =0; i < c; i++) {
       client.query(req, function(err) {
          next();
       });
   }
};


test(function() {
   var time2 = new Date().getTime();
    console.log(t2-t);
   client.end();
});

Why it's almost the same time ? Since it's mysql we don't gain that much from the "async" from node-js in this situation. See comments.

edited Nov 24, 2011 at 4:01

answered Nov 24, 2011 at 1:13

malletjo

1,78616 silver badges19 bronze badges

8 Comments

Raynos Over a year ago

"recursive pattern" is wrong, that's not asynchronous, that's waterfall and slow as hell.

malletjo Over a year ago

Good point. I would not say slow as hell ... but reference counting is better, a lot

Raynos Over a year ago

recursion means only one task is running at a time. This means zero parallelism which means slow as hell

Raynos Over a year ago

Your benchmark is broken, don't know why

Raynos Over a year ago

I see why your code doesn't go faster. It's because your using mysql and it's using a transaction. So even though the reference counting code sends 100 mysql requests in parallel, mysql only processes it one at a time. This is why we use a real database and it's a good example of how one piece of blocking code (mysql) becomes your entire application bottleneck

|

alessioalex · Accepted Answer · 2011-11-24 09:05:46Z

You aren't doing this properly since the server could respond before the db queries have returned their results.

You should instead use control flow libraries like Step or Async. Learn more about them on dailyjs.

I personally use Step, so here's show I would do it:

app.get('/deck', function(request, response) { 
  dealer.sort('cards', 'BY', 'deal:*->timestamp', 'GET', 'card:*->card_key',
    function(err, card_keys) {
      var deals = [], iterations = Object.keys(card_keys).length;
      // iterations contains the length of 'card_keys'
      Step(
        function queries() {
          var deck = [];
          if (!iterations) { this(null); return; } // if no cards
          for (k in card_keys) {
            dealer.hget(card_keys[k], 'data', function(err, card_data) {
              var deal;
              if (err) {
                throw new Error('Database error');
              } else {
                // we decrease the number of iterations until it gets to 0
                // and then we trigger the callback, since every query has
                // returned its result
                iterations--;
                deal = eval('(' + card_data +')');
                deals.push(cards);
                if (!iterations) { this(deals); } // callback function called
              }
            });
          }
        },
        function render(deals) {
          response.json(deals);
        }
      );
    }
  );
});

Collectives™ on Stack Overflow

Redis, node.js and Javascript Callback Scoping / Flow

3 Answers 3

3 Comments

8 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

3 Comments

8 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related