JS Closures, Redis, loop, Async :: empty array

Question

I give up on this. May some of the wise stackoverflow monks please fix my bugs?

Code is self explaining. Client sends room names, server does a redis lookup and pushes valid rooms to the array. After adding all the rooms, the list should be emitted to the client.

Problem is closure, async etc. based. I understand the problem but cannot get a workaround because the array needs to remain inside the function. Tricky.

Code:

function roomList(socket){

  var roomlist = [], rooms = getRooms(), p = /pChannel_/;

  redis.select(7, function(err,res){

    for (var k in rooms){

      if(rooms[k] != '' && p.test(rooms[k])){

        var key = 'channel:'+rooms[k];

        redis.hgetall(key, function (err, reply) { 

          if(reply){ 
            var c = io.sockets.manager.rooms[rooms[k]];
            roomlist.push( Array(reply['name'],c.length,reply['icon']) );
          }
          else { console.log('nothing found'); }

        });

      }

    }

    // here be dragons
    console.log(roomlist);
    socket.emit('roomList', roomlist);

  });

}

Thanks.

Didier Spezia · Accepted Answer · 2013-06-16 10:08:29Z

1

C'mon guys. The OP explicitly said she/he is interested by understanding how things are supposed to work. And you don't need Q or async or any other 3rd party modules to implement this.

In the initial code, there are two problems:

with Javascript, the closure scope is at function level, not block level. A function must be introduced to define a proper closure. Here, a simple forEach can be used.
the final step (i.e. emit) is not run after the replies have been received from Redis. It must be called in the loop itself. In order to achieve it, it is required to count the items so that the inner callback can test whether the process is complete or not.

So here is another version:

function roomList(socket){

  var roomlist = [], rooms = getRooms(), p = /pChannel_/;

  redis.select(7, function(err,res){
    var count = rooms.length
    rooms.forEach( function(r) {
      if( r != '' && p.test(r) ) {
        var key = 'channel:'+r
        redis.hgetall(key, function (err, reply) { 
          if(reply) { 
            var c = io.sockets.manager.rooms[r];
            roomlist.push( Array(reply['name'],c.length,reply['icon']) );
          } else {
            console.log('nothing found');
          }
          if ( --count <= 0 ) {
            // here be dragons
            console.log(roomlist);
            socket.emit('roomList', roomlist);
          }
        });
      } else --count;
    });
  });
}

answered Jun 16, 2013 at 10:08

Didier Spezia

73.6k12 gold badges194 silver badges157 bronze badges

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

9 Comments

user2429266 Over a year ago

You guys are kidding?^^ Is counting not a "hack around"? Sure it solves the problem..

Didier Spezia Over a year ago

Well, personally, this is what I would use, and this is pure continuation passing style, without relying on any fancy libraries.

Beetroot-Beetroot Over a year ago

I might have used a count approach 5 or 6 years ago but nowadays, no way - at least not unless I was backed into a cold dark corner.

robertklep Over a year ago

@DidierSpezia your solution doesn't preserve order, which might – or might not – be a problem. Personally, I find keep track of counts to lead to confusing code. Also, OP states that he understands the problem but is struggling to find an answer. We provided one.

user2429266 Over a year ago

Something like this doesn't work? codereply = function() { return redis.hgetall(key, function (err, reply) {return reply}) }code @robertklep The problem is getting the return value from the redis.get function out of it's closure. So counting must be considered as a hack. EDIT: Ok. Not defined in my post.

|

robertklep · Accepted Answer · 2013-06-16 08:38:23Z

1

Looks like a job for async.map:

function roomList(socket){
  var rooms = getRooms(), p = /pChannel_/;

  redis.select(7, function(err, res) {
    async.map(rooms, function(room, callback) {
      if (room === '' || ! p.test(room))
        return callback(null, null);

      var key = 'channel:' + room;
      var c   = io.sockets.manager.rooms[room];
      redis.hgetall(key, function (err, reply) {
        if (err)
          callback(err); // propagate Redis errors to final callback, don't know
                         // if you want that or not; use 'callback(null)' if not.
        else
        if (reply)
          callback(err, Array(reply.name, c.length, reply.icon) );
        else
          callback(err, null);
      });
    }, function(err, roomlist) {
      if (err)
         // handle Redis errors...

      // filter 'null' entries from roomlist
      roomlist = roomlist.filter(function(room) { return room !== null });
      console.log(roomlist);
      socket.emit('roomList', roomlist);
    });
  });
}

(untested)

answered Jun 16, 2013 at 8:38

robertklep

205k38 gold badges421 silver badges412 bronze badges

1 Comment

user2429266 Over a year ago

Looks like a good solution but that would just ship me around the process of understanding. I would prefer ways without external methods like map or waterfall. Some pseudocode/hints are fine.

Beetroot-Beetroot · Accepted Answer · 2013-06-16 09:36:43Z

0

If you just want to wait for the room list to be fully built before emitting the response (as seems highly reasonable), and assuming Q to be available, then you should just need a few additional lines of Q magic plus a closure-forming wrapper around the inner code to maintain a reliable reference to a Q deferred at each pass of the for loop.

function roomList(socket) {
    redis.select(7, function(err, res) {
        var list = [],
            rooms = getRooms(),
            p = /pChannel_/,
            promises = [];
        for(var k in rooms) {
            if(rooms[k] != '' && p.test(rooms[k])) {
                (function(dfrd) {
                    promises.push(dfrd.promise);
                    var key = 'channel:' + rooms[k];
                    redis.hgetall(key, function(err, reply) {
                        if(reply) {
                            var c = io.sockets.manager.rooms[rooms[k]];
                            list.push( [reply['name'], c.length, reply['icon']] );
                        }
                        else {
                            console.log('nothing found');
                        }
                        dfrd.resolve();
                    });
                })(Q.defer());
            }
        }
        Q.all(promises).then(function() {
            console.log(list);
            socket.emit('roomList', list);
        });
    });
}

answered Jun 16, 2013 at 9:36

Beetroot-Beetroot

18.1k3 gold badges39 silver badges44 bronze badges

2 Comments

user2429266 Over a year ago

No Q available. But I see that this situation is a bit more tricky then what I have thought.

Beetroot-Beetroot Over a year ago

You will struggle without Q or similar.

Collectives™ on Stack Overflow

JS Closures, Redis, loop, Async :: empty array

3 Answers 3

9 Comments

1 Comment

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

9 Comments

1 Comment

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related