When/Node doesn't liftAll on Redis

[ghost]

I'm just starting in with when.js, so I'm 90% sure I'm just doing something wrong, but for the life of me I can't figure it out (or find it mentioned elsewhere). Please point me to a better forum for asking for help if I'm in the wrong place.

My problem is that I'm trying to use when.js with the redis module to promis-ify its API. However, when I follow the guide for node.liftAll, it appears as though all the methods on the returned object do exactly what the methods did on the original.

Here are some repro steps and example code:

STEPS

1. Install Redis server locally (e.g., apt-get install redis)
2. Install "when" and "redis" node modules (e.g., "npm install redis && npm install when")
3. Drop this file on your system as example.js:

when = require("when/node");
redis = require("redis");

client = redis.createClient(6379, "127.0.0.1");
client_when = when.liftAll(client);

console.log("client.get === client_when.get ?  " + (client.get === client_when.get));

process.exit();

1. Run it (e.g., node example.js)

   EXPECTED

The program will output client.get === client_when.get ? false because the get method has been lifted on one, but not on the other.

ACTUAL

The program outputs client.get === client_when.get ? true. Further, if you attempt to actually call the get method on either object, they both function exactly like the original method.

[briancavalier]

Thanks for such a detailed report, @redwood-labs!

Right now, liftAll lifts only owned properties and not inherited properties (it uses Object.keys). It looks like redis.createClient creates an object from a prototype, so all of its methods are inherited and not own methods.

We could consider allowing liftAll to lift inherited methods as well.

Inheriting to create your own lifted prototype would be a good option. I'm not super-familiar with the redis package, but here's a quick example of how you might do it:

var node = require('when/node');
var RedisClient = require('redis').RedisClient;

function PromisedRedisClient() {
   RedisClient.apply(this, arguments);
}

PromisedRedisClient.prototype = node.liftAll(RedisClient.prototype);

var client = new PromisedRedisClient(...);

var promise = client.get(...);

Does that help?

[ghost]

Thanks for the excellent and timely response, Brian! I'm afraid I still can't get things working, though. I've declared the new Redis class (require's omitted) as:

function PromisedRedisClient() {
    RedisClient.apply(this, arguments);
}
PromisedRedisClient.prototype = whenNode.liftAll(RedisClient.prototype);
connection = net.connect(config.port, config.host);
exports.client = client = new PromisedRedisClient(connection);
client.port = config.port;
client.host = config.host;

And then, I have a unit test (using Mocha) which attempts to make a simple call to the database:

describe('sample', function() {
  it('should work', function(done) {
    redis.get("foo").done(function(value) {
      value.should.equal('bar');
      done();
    }, function(error) {
      fail(error);
    });
  });
});

The unit test fails because the done() method hasn't been called within 2s.

Just as a quick sanity check, I also wrote this little chunk of code using the original functions, and found that it works properly:

redis = require('redis');
client = redis.createClient();
client.get('foo', function(error, value) {
  console.log("value: " + value);
});

Any other ideas of what I might be going on here?

[ghost]

I should also mention a few other things I learned:

• The new get function has definitely been lifted. Printing it to the console shows the contents of the returned function from when/node.js#lift.
• The Redis log shows that the client correctly connected, but never issued any commands.

[briancavalier]

The new get function has definitely been lifted

Hmmm, yes, this is certainly a clue. So liftAll seems to be doing it's part of the job, but the lifted method isn't doing the right thing. node.liftAll lifts node-style async functions where the last parameter is supposed to a node-style callback function(err, result) {...}. If the function deviates far from that "standard", lift won't work. So one possibility is that RedisClient's methods do something outside those bounds.

I'll take a look at their API and code to see if that might be the case. Hopefully have more info for you soon.

[briancavalier]

Hmmm, liftAlling a prototype might just be tricky business. For example, if get calls methods on itself like this.whatever(), then they may have been lifted, so might break the object's internal assumptions.

An experiment you could try is using liftAll's optional combine function to rename methods so as not to overwrite the originals, or maybe only lift the part of the API you care about.

For example:

// Don't overwrite original names, instead name the lifted functions <original>Async, eg getAsync
PromisedRedisClient.prototype = node.liftAll(RedisClient.prototype, function(proto, liftedFunc, name) {
    proto[name + 'Async'] = liftedFunc;
    return proto;
});

// or only lift some public methods you care about
PromisedRedisClient.prototype = node.liftAll(RedisClient.prototype, function(proto, liftedFunc, name) {
    if(name in setOfNamesICareAbout) {
        proto[name] = liftedFunc;
    }
    return proto;
});

You can combine renaming and filtering as well.

If you try it, let me know what you learn. This is all valuable information. Since liftAll is fairly new, it's good to see various real world use cases that we haven't tried with it, so that we can refine/improve it!

[briancavalier]

Ok, after a quick debugging session, I think it is the self method calls that are the problem. Lots of the public methods like get on RedisClient are generated here. Almost immediately, they call this.send_command, which will have been lifted.

I also talked to @phated, who has used liftAll and redis together successfully. He confirmed that renaming the methods (he added 'Async' to the end of them) works for him.

Let me know if that works for you.

[ghost]

Hooray! I got something working! I decided to go with the suggestion to just lift those functions I needed to, and simplified my Redis adapter code down to:

exports.client = client = redis.createClient config.port, config.host
for func in [ 'get', 'set', 'hgetall', 'hmset' ]
    client[func] = whenjs.lift client[func]

This avoided the need to mess with the prototypes or subclasses altogether at the cost of having to specify the functions manually. I suspect I'll find out the dozen or so functions I care about and that will be that, however. That makes my test pass:

describe 'Redis', ->
    describe 'basic functions', ->
        it 'get, set, del', (done)->
            redis.set('foo', 'bar')
            redis.get('foo').done(
                (value)->
                    value.should.equal 'bar'
                    done()
                , (error)->
                    fail(error)
            )

Thanks a lot for your help, Brian!

[briancavalier]

@redwood-labs Nice solution to selective lift! We are thinking on the same frequency too ... I just had a discussion w/@phated on irc where we came up with the idea to lift only the set of command methods as defined in redis/lib/commands.

Here's a gist of what we came up with. It has 2 variants. The first goes the prototype route, and the second lifts an instance. Seems like it could be a good, general solution.

Anyway, glad you've got it working. I think I'll open an issue to add some examples to the docs.

[sompylasar]

Are you sure Redis does not internally call the methods you lifted in some methods you haven't yet tested? I'd stick with adding a suffix to all lifted methods and use them instead because the methods signatures should not change. You could have used other libraries which assume the original methods' signatures and which would break, too.

[ghost]

@sompylasar I'm not 100% sure, but from what I can tell the final end-user methods are all generated and there are private methods which are used internally. If I can get away with it, it would be nice not to have to change the names, but if not, then I don't mind doing so. It will be the first thing I look at if I start getting mysterious failures.

[ghost]

@briancavalier That's a good idea, but I think you may need to be more selective than that. There are a bunch of methods (e.g., set) which don't accept a callback function and therefore don't need to be lifted. You can probably come up with a regex that defines which ones need the lift.

[briancavalier]

@sompylasar @redwood-labs Yeah, you guys are right--a 100% general solution is nigh impossible when there are signature variations, inherited vs. own methods, etc etc. That's another good reason to rename. Then, the caller can decide to call getAsync and set.

[bertho-zero]

For liftAll commands of redis you can do:

"use strict";

const wn = require( 'when/node' );
const commands = require( 'redis-commands' ).list;

module.exports = client => {

  for ( const name of commands ) {

    client[ name + 'Async' ] = wn.lift( client[ name ] ).bind( client );

  }

  return client;

}

And use it as:

const client = redis.createClient();

promisifyRedis( client );