@soloestoy it is assumed that the developer will not set a NULL timeout function with a real timeout value. I don't think we should protect from it, especially not in that way.

Yes, but I think it is necessary because we allow developers to set NULL timeout function, and we already check the reply_callback function in

void moduleHandleBlockedClients(void) {
    ...
       if (c && bc->reply_callback) {
    ...
}

Why don't we check timeout_callback?

Is it better to only allow timeout function be NULL when timeout is zero in the API RedisModule_BlockClient? @dvirsky

@dvirsky another question, it seems that we allow developers call RedisModule_Call with "!" flag to replicate commands in reply_callback, see function moduleHandleBlockedClients:

void moduleHandleBlockedClients(void) {
    ...
        if (c && bc->reply_callback) {
            RedisModuleCtx ctx = REDISMODULE_CTX_INIT;
            ctx.flags |= REDISMODULE_CTX_BLOCKED_REPLY;
            ctx.blocked_privdata = bc->privdata;
            ctx.module = bc->module;
            ctx.client = bc->client;
            bc->reply_callback(&ctx,(void**)c->argv,c->argc);
            moduleHandlePropagationAfterCommandCallback(&ctx);
            moduleFreeContext(&ctx);
        }
    ...

Do we allow developers do replication in timeout_callback too? I didn't find the forbidden rule, so I think it's necessary to call moduleHandlePropagationAfterCommandCallback after timeout_callback too.

void moduleBlockedClientTimedOut(client *c) {
    RedisModuleBlockedClient *bc = c->bpop.module_blocked_handle;
    RedisModuleCtx ctx = REDISMODULE_CTX_INIT;
    ctx.flags |= REDISMODULE_CTX_BLOCKED_TIMEOUT;
    ctx.module = bc->module;
    ctx.client = bc->client;
    if (bc->timeout_callback) {
        bc->timeout_callback(&ctx,(void**)c->argv,c->argc);
        moduleHandlePropagationAfterCommandCallback(&ctx);
    } else {
        addReply(c,shared.nullmultibulk);
    }
    moduleFreeContext(&ctx);
}

Hello @soloestoy, @dvirsky,

to set a NULL callback with non-zero timeout is indeed an API misuse, however what we could do to fix this instead is to just register a default timeout callback if NULL is passed. One just replying "-ERR blocked module command timed out, but timeout callback is NULL". This will be much more evident and does not need any extra check.

About the second fix, it's odd that people could do actual work in the timeout callback, but there is also no easy way to prevent it, so I think that you fix makes sense here: at least it does not break replication in that case, so I'm merging 5021e3c.

About the other fix, if you agree with the fix, you can write the default timeout handler inside module.c and set it in case it's NULL (even if the timeout it's 0, you never know if the internals could call the function in the future). Thanks for your help!

No sorry can't merge just one, the two touch the same code. I'll wait for some update of the PR. Thanks.

OK, I will update this PR later.

PR updated, please check @antirez

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}