@sjpotter can we add a test for that specific case as you mentioned here

trying to work on it right now, trying to figure out, how to get it to work in tcl, my command filter (added to commandfilter.c) is

/* Filter to protect against Bug #11894 reappearing */
void CommandFilter_BlpopFilter(RedisModuleCommandFilterCtx *filter)
{
    if (RedisModule_CommandFilterArgsCount(filter) != 3)
        return;

    const RedisModuleString *arg = RedisModule_CommandFilterArgGet(filter, 0);
    size_t arg_len;
    const char *arg_str = RedisModule_StringPtrLen(arg, &arg_len);

    if (arg_len != 5 || memcmp(arg_str, "blpop", 5))
        return;

    blpop_count++;

    if (blpop_count % 2 != 0)
        return;

    RedisModule_CommandFilterArgInsert(filter, 0, RedisModule_CreateString(NULL, "--inserted-before--", 19));
}

actually, blpop isn't a good case, as it always gets timeout from last arg. need to use brpoplpush as it gets it from a fixed position arg.

actually, blpop isn't a good case, as it always gets timeout from last arg. need to use brpoplpush as it gets it from a fixed position arg.

you can also use BLMPOP... easier still

added a test, but while my new test passes, older existing tests are seeing failures, unsure why at the moment.

@sjpotter I wonder if we can conclude that it's always wrong to call the filter on reprocessing, as it depends on what the filter tries to achieve. Some ideas:

• We can add a REDISMODULE_CMDFILTER_ON_UNBLOCK to indicate the callback should also be called on unblock.
• We can also extend the filter context with a flag of whether or not we're reprocessing and add an API function to expose this.

@yossigo, don't necessarily disagree with your post, some other things that could be added.

1. extend filter context to indicate where the cmd is coming from. i.e. is it coming from a module (which one)? or from normal command processing? would deprecate the need for NOSELF, and simplify redis logic as the callback can implement it itself.

2. as you said, is it do to being reprocessed?

3. has it been filtered already (i.e. perhaps get both original state.

With all that said, what is the logic for running it again on reprocess? i.e. on first time through, the cmd did or didn't get modified, "reprocess" is an internal redis implementation detail, why couldn't it be implemented differently. Lets say blocking instead was handled at the blpopCommand level, it wouldn't ever reprocess. I'm trying to come up with a use case where the user would want to do that.

With that said, I'm not against the idea of being overly more flexible (and as noted, give ideas of more than can be done).

I agree with @sjpotter that the reprocessing is an internal step. this is more compatible with the way it was handled previously (before #11012)

i personally agree with the above. while we can do all these things, we don't have a need for that yet, and we can keep it working the same as it was before we change the internals.
@yossigo please ack.

I've discussed this with Yossi, we agree that the extensions described above can / should be postponed to some future date, and for now we just need to make sure the internals change doesn't break previous behavior.
i'll review the test here soon and we can merge it.

@sjpotter i saw some missing comments in your command filter PR and decided to edit them instead of bothering you.
but then i realized maybe i'm missing something, so i added some comments, please clean it up.