callbacks

[dvv]

Hi!

mranney's node_redis supports optional callbacks. However, the logic of determining whether the trailing undefined means "no callback", or this undefined means the last argument to a multi-argument redis command, is ambiguous.

Suppose we call List::next(). The resulting command passed to redis client's send_command is .lpop with arguments ['id-of-the-list', undefined]. This undefined is treated as argument to lpop and chokes the client. This pattern is widely used in redback's methods.

I propose to change the pattern to always wrap real command arguments in an array, which is supported by redis client as well and can even be faster (tests?). That way redis client is always given two arguments -- array of args ready to be applied, and truly optional callback.

update: in fact, #2 is an illustration of the issue.

update2: to clarify what i mean -- that's how to easily cope with optional callbacks: https://github.com/muchmala/rq/blob/master/lib/clients/storage.js

TIA, --Vladimir

[chriso]

This is probably something I should fix. I started making some callbacks optional (here, here and here) but it's a bit tedious going down this route.

I've contacted Matt, the author of node-redis, re the issue - there might be another workaround

[dvv]

Thanks! Once callbacks are separated, we wouldn't care of checking whether it's defined -- this would make the code clearer, robuster, and faster, imho.

[mranney]

Hey guys, internally node_redis tries to be helpful and let you call send_command with either a series of arguments, or an array of arguments. Sometimes this logic is tricky to follow and get right, so I'm hesitant to change it, unless it is less likely to be surprising to users. The end result though is that send_command wants to stash an object like this:

command_obj = {
    command: command,
    args: args,
    callback: callback,
    sub_command: false
};

If args comes in as an Array already, then I don't need to convert it. If it's not too hard for you guys, I think the best way is to send in an Array of args.

However, I'm open to suggestions.

[dvv]

This coincides with the proposal. @chriso, please consider

[chriso]

Thanks Matt.

@dvv the issue comes from the way I've designed the API, specifically with optional parameters. For example, the list.shift() method takes (wait, callback) - currently you can call the method to shift immediately list.shift(callback), or to block for a specified amount of seconds waiting for input list.shift(wait, callback).

The method from your 2nd update works well if the number of parameters is fixed and callback is either function or undefined, however this isn't the case with many of the methods in Redback.

To implement optional callbacks in all methods I'd either have to redesign the API (which I'm not prepared to do at this stage) or add extra logic to every method to determine which parameters were passed - e.g. all of the following would be valid list.shift calls with the proposed change: list.shift(wait), list.shift(callback), list.shift(wait, callback), list.shift() - the number of cases grows exponentially based on how many optional parameters there are.

I've manually added callback || function(){} to some methods and marked them as optional in the API. The only methods that require a callback are redis calls that return something - e.g. there's no reason why you would ever call list.length without a callback..

For now, I'm going to leave it as is. It's trivial for a user to pass a noop as a callback. If there's any methods in particular that you think should have optional callbacks, let me know and I'll manually set a noop.

[dvv]

I see. Let's reconsider List::next() which is perfectly valid case i suppose. The resulting command passed to redis client's send_command is .lpop with arguments ['id-of-the-list', undefined]. undefined as the last argument is pushed solely due to redback's logic of shifting parameters. It shouldn't just ever go to arguments list. If it were not passed down to redis layer, redis wouldn't take it as a parameter. Because redis treats specially the last parameter iff it's of type function.