Redis [BUG] Calling RedisModule_SignalKeyAsReady from a timer callback does not trigger the reply callback.

Describe the bug

When using RedisModule_BlockClientOnKeys it should be possible to unblock the client by calling RedisModule_SignalKeyAsReady. However this is not working if the call is performed inside the callback of a timer. The following code reproduces the issue. Note that for RedisModule_SignalKeyAsReady to work at all, the key we are signalling must exist before we call it as reported here (https://github.com/redis/redis/issues/7878)

#include "redismodule.h"
#include <stdlib.h>

int BlockedReplyCallback(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)  {
    RedisModule_Log(ctx, "warning", "Reply callback!");
    RedisModule_ReplyWithSimpleString(ctx, "We are done!");
    return REDISMODULE_OK;
}

int TimeoutReplyCallback(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)  {
    RedisModule_Log(ctx, "warning", "Timed out!");

    RedisModule_ReplyWithSimpleString(ctx, "Timed Out");
    return REDISMODULE_OK;
}

void BlockedFreePrivdata(RedisModuleCtx *ctx, void *privdata) {
    RedisModule_Log(ctx, "warning", "Freeing all priv data");
}

void TimerCallback(RedisModuleCtx *ctx, void *data) {
    RedisModuleString *key = data;

    RedisModule_Call(ctx, "RPUSH", "sc", key, "foobar");
    RedisModule_SignalKeyAsReady(ctx, key);
}

int HelloworldRand_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {

    RedisModuleBlockedClient* bc = RedisModule_BlockClientOnKeys(ctx, 
            BlockedReplyCallback, 
            TimeoutReplyCallback, 
            BlockedFreePrivdata,
            10000, 
            &argv[1],
            1, 
            NULL);

    RedisModule_HoldString(ctx, argv[1]);
    RedisModule_CreateTimer(ctx, 
                1000, 
                TimerCallback, 
                argv[1]);

    return REDISMODULE_OK;
}

int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
    if (RedisModule_Init(ctx,"helloworld",1,REDISMODULE_APIVER_1)
        == REDISMODULE_ERR) return REDISMODULE_ERR;

    if (RedisModule_CreateCommand(ctx,"bug2",
        HelloworldRand_RedisCommand, "",
        0, 0, 0) == REDISMODULE_ERR)
        return REDISMODULE_ERR;

    return REDISMODULE_OK;
}

To reproduce

Just compile and load that module in redis, call it with for example "bug2 1234". It should create a list named 1234 with one element "foobar" in it. However the command is not unblocked after 1 second.

Expected behavior

The command should unblock one second after running it.

Additional information

This issue is reproducible with Redis 6.0.8.

Some observations. If the call to RPUSH is moved outside of the timer callback, then the command will unblock the first time it is called, but not the second time (as if it detects the LIST was already there and then ignore the signalling).

Comment From: guybe7

@manast it seems you pass argv to RedisModule_CreateTimer but as soon as HelloworldRand_RedisCommand finished argv is not a valid pointer anymore... maybe just pass argv[1] (after retaining it)

let me know if the issue is still happening

Comment From: manast

I updated the test code, it is still happening.

Comment From: guybe7

i still see

RedisModule_CreateTimer(ctx, 
                1000, 
                TimerCallback, 
                argv);

instead of

RedisModule_CreateTimer(ctx, 
                1000, 
                TimerCallback, 
                argv[1]);

Comment From: manast

not anymore :). I just forgot to update that in the issue, in my code it is correct.

Comment From: manast

but as I mentioned, if you push before creating the timer then the call to signalKeysAsReady works, so the pointer must be correct, the problem is when you push from inside the timer AND you call the signalKeyAsReady from inside the timer.

Comment From: guybe7

ok so indeed there's a bug: the function that releases blocked clients (handleClientsBlockedOnKeys) is called only from processCommand (i.e. the assumption is that only a command can cause a key to be "ready" - but that assumption seems to be an over-simplification)

we can either call handleClientsBlockedOnKeys at the end of moduleTimerHandler or every once in a while in serverCron (seems a bit more risky)

@oranagra WDYT?

Comment From: oranagra

i think this was indeed overlooked, and i guess we should call handleClientsBlockedOnKeys from beforeSleep too.

Comment From: oranagra

handled vie the above mentioned PR.