send_command("monitoring") is doomed to fail

[bpytlik]

On line 360 of index.js the code says

this.send_command("monitor")

In the send_command function, examine line 735 and line 756. If args is not an array, we throw the error on line 756. Since only the string "monitor" was passed on line 360, we're guaranteed to hit this error.

I think this is preventing me from monitoring more than one redis instance in the same process space, even though I'm using different clients. Regardless of whether I'm doing something else dumb (definitely possible), the code as written can't work I think.

[bpytlik]

This seems to fix my issue. Any chance this can be pulled in soon?

[bpytlik]

This issue still exists, though it's now at line 372 (in master as I write this). Please reopen this issue. Also, please accept the pull request that fixes this issue and has been sitting there for 3 months.

[blainsmith]

I haven't been able to reproduce this error. I am in the middle of writing a test for this command as well.

[bpytlik]

The issue exists by inspection. Here is the line in question:

this.send_command("monitor");

Here is the send_command function:

RedisClient.prototype.send_command = function (command, args, callback) {
    var arg, command_obj, i, il, elem_count, buffer_args, stream = this.stream, command_str = "", buffered_writes = 0, last_arg_type, lcaseCommand;

    if (typeof command !== "string") {
        throw new Error("First argument to send_command must be the command name string, not " + typeof command);
    }

    if (Array.isArray(args)) {
        if (typeof callback === "function") {
            // probably the fastest way:
            //     client.command([arg1, arg2], cb);  (straight passthrough)
            //         send_command(command, [arg1, arg2], cb);
        } else if (! callback) {
            // most people find this variable argument length form more convenient, but it uses arguments, which is slower
            //     client.command(arg1, arg2, cb);   (wraps up arguments into an array)
            //       send_command(command, [arg1, arg2, cb]);
            //     client.command(arg1, arg2);   (callback is optional)
            //       send_command(command, [arg1, arg2]);
            //     client.command(arg1, arg2, undefined);   (callback is undefined)
            //       send_command(command, [arg1, arg2, undefined]);
            last_arg_type = typeof args[args.length - 1];
            if (last_arg_type === "function" || last_arg_type === "undefined") {
                callback = args.pop();
            }
        } else {
            throw new Error("send_command: last argument must be a callback or undefined");
        }
    } else {
        throw new Error("send_command: second argument must be an array");
    }

So in the call to send command, we pass a string and no arguments. In send_command, if the second argument does not exist, then we throw an error "send_command: second argument must be an array".

To reproduce this, stand up a redis server. Create two clients (in a single process) and have them each try to monitor the server. If that doesn't reproduce the issue, try creating 2 redis servers, creating 2 clients (again, in a single process) and have each monitor one of the redis servers.

As a side note, the entire fix for this bug is a 4 character change, including white space and exists in a pull request. Please just take the PR.

[blainsmith]

I have requested that the author add tests to the PR since we recently updated the test suite.

Thanks for the details on the reproduction.

[BridgeAR]

@bpytlik send_command will also accept undefined args from v.2 on :)