Describe the bug

In working with https://github.com/redis/redis/pull/11568, I have a module that I was issuing a brpoplpush in blocking mode, but also filtering it to add an argument to the front of the command (so it was no longer a brpoplpush, but something I'd wrap and issue an underlying brpoplpush).

But, I was getting an error that it had an invalid timeout. In inspecting it, it seemed it was trying to calculate a timeout on what would have been the argv[2] (i.e. dest key), but it was now at argv[3].

was really confused, till @MeirShpilraien figured out when I mentioned command filter.

the existing code does

    moduleCallCommandFilters(c);

     /* in case we are starting to ProcessCommand and we already have a command we assume
      * this is a reprocessing of this command, so we do not want to perform some of the actions again. */
     int client_reprocessing_command = c->cmd ? 1 : 0;

the problem is below, the code does

    if (!client_reprocessing_command) {
        c->cmd = c->lastcmd = c->realcmd = lookupCommand(c->argv,c->argc);
    ...
    }

i.e. it cached the lookup.

For progress purposes I changed the code to 

     /* in case we are starting to ProcessCommand and we already have a command we assume
      * this is a reprocessing of this command, so we do not want to perform some of the actions again. */
     int client_reprocessing_command = c->cmd ? 1 : 0;

    /* only run command filter if not reprocessing command */
    if (!client_reprocessing_command) {
        moduleCallCommandFilters(c);
    }

and caching the lookup is now fine.

but that's not necessarily the correct fix.

The Q to answer is, should the command filter be run when a command is reprocessed. If the answer is yes, we can't cache the lookup. If a command that is reprocessed should run exactly as it did before, then the answer is that it shouldn't be run through the command filter and caching is fine.

My feeling is that caching is fine, and shouldn't run through command filter again, but perhaps there are use cases where it would need to (But then can't ever cache, at least if run through a filter, and then would have to modify filter callbacks to indicate if they modified the argv or not)

To reproduce

a module that uses a command filter to change execution of a command that will block.

Expected behavior

described above, but to repeat

The Q to answer is, should the command filter be run when a command is reprocessed. If the answer is yes, we can't cache the lookup. If a command that is reprocessed should run exactly as it did before, then the answer is that it shouldn't be run through the command filter and caching is fine.

Comment From: oranagra

i tend to agree with you that we should not re-run the command filter. but maybe i'm not aware of all the potential use-cases for the command filter. i.e. other than modifying the command, what else did we intend for it to do? @yossigo ?

Comment From: MeirShpilraien

I also agree we should not rerun the filter. We run it already.

Comment From: ranshid

I also agree this sounds reasonable. @sjpotter do you want to create a PR or should I?

Comment From: sjpotter

I'll submit what I have for now. We should probably make a TODO for after @MeirShpilraien PR lands of a test that has a module calling a blocking command with a commandfilter to ensure this doesn't break in future as well. I'm unsure how to duplicate it without the blocking RM_Call support.

Comment From: MeirShpilraien

I'm unsure how to duplicate it without the blocking RM_Call support.

Create a filter that will change each second blpop command and will set invalid timeout. Then the first blpop will run OK and once unblocked make sure that it is not getting invalid timeout (the filter did not run). I know there is no real reason to write such module, but it will serve the purpose of making sure this will not break in the future.