[BUG] RedisModule_UnblockClient behaviour does not match the documentation

[manast]

Describe the bug
sorry if this text is a bit confusing, it has been many hours of debugging until I realized this may be an issue with redis.

The documentation for RedisModule_UnblockClient is either incorrect or something does not add up in how to use the function when blocking a client with RedisModule_BlockClientOnKeys.

Due to issue #7878 I am using the function above as a workaround for unblocking the client. In this case the client is unblocked as expected, and this time as the documentation states (confusingly since it first talks about reply callback then clarifies that it is the timeout callback) it calls the Timeout callback instead of the reply callback (https://redis.io/topics/modules-api-ref)

Note 2: when we unblock a client that is blocked for keys using the API RedisModule_BlockClientOnKeys(),
the privdata argument here is not used, and the reply callback is called with
the privdata pointer that was passed when blocking the client.

Unblocking a client that was blocked for keys using this API will still require
the client to get some reply, so the function will use the "timeout" handler in
order to do so.

The problem is that the signature of the timeout callback is this one (in zig):

fn TimeoutReplyCallback(ctx: ?*RedisModuleCtx, argv: [*c]?*RedisModuleString, argc: c_int) callconv(.C) c_int

There is no privdata argument in the signature, so that must be a mismatch with the documentation. But the biggest problem is that if you try to get the privdata with

    const pData: *ReplyData = @ptrCast(*ReplyData, RedisModule_GetBlockedClientPrivateData.?(ctx));

pData will always be NULL. Therefore it becomes impossible to access the privdata in this scenario.

I am missing something?

[guybe7]

hello @manast
there are two different mechanisms for blocking a client:

1. "regular" blocking: the module blocks a client, does some processing in a different thread and unblocks the client using RM_UnblockClient
2. "key" blocking: the module blocks a client pending a specific state of a key. when the key reaches some desired state RM_SignalKeyAsReady should be called and the client is unblocked

in the first case, when you block the client by calling RM_BlockClient (which doesn't have a privdata argument). when you unblock using RM_UnblockClient you provide a privdata which is accessible in the reply_callback via RedisModule_GetBlockedClientPrivateData

in the second case you call RM_BlockClientOnKeys and you may provide a privdata. if you choose to unblock the client with RM_UnblockClient (which also takes a privdata) none of the privdata is used and the client is released with the timeout_callback (the logic is that if you chose to unblock a blocked-on-keys-client with RM_UnblockClient instead of RM_SIgnalKeyAsReady it means that something is wrong)

general comment: the privdata (both the one provided in RM_UnblockClient and optionally in RM_BlockClientOnKeys) is not accessible in the timeout_callback (only in the reply_callback and disconnect_callback)

so yes, there is a problem with the docs. the section you sent should be corrected to:

Note 2: when we unblock a client that is blocked for keys using the API RedisModule_BlockClientOnKeys(),
the privdata argument here is not used.
Unblocking a client that was blocked for keys using this API will still require
the client to get some reply, so the function will use the "timeout" handler in
order to do so. Please note that both the privdata provided here and the one provided to
RedisModule_BlockClientOnKeys are not accessible from the timeout_callback.

we should change the doc of RedisModule_GetBlockedClientPrivateData:

Get the private data set by RedisModule_UnblockClient()
Note: This API will return NULL if called from within the timeout_callback.

[manast]

yeah, basically I knew already all you wrote above after many tests and investigations. The problem is that as I show in #7880 RM_SignalKeyAsReady does not work when called from a timer callback or from a thread (probably the timer callback is called from a thread too it is my guessing). So I am forced to use RM_UnblockClient and of course is a bit inconvenient not having access to the privdata anymore.

[guybe7]

it seems to me the "problem" here is that there's no access to privdata from the timeout callback (after #7880 is cleared that won't be a problem for you as you will not need to call RM_UnblockClient any more)

TBH i don't really see an advantage on accessing privdata in the timeout_callback... after all, it's just a timeout, why would you need privdata? @oranagra thoughts?

[oranagra]

I think it's always better if any callback would get a privdata.
question is if we can do that easily, and without breaking backwards compatibility.

[guybe7]

yes it's very simple, we just need to assign ctx->blocked_privdata before calling timeout_callback

[manast]

TBH i don't really see an advantage on accessing privdata in the timeout_callback... after all, it's just a timeout, why would you need privdata? @oranagra thoughts?

Normally it is a timeout but when you call UnblockClient manually even though it calls Timeout it has been called before the actual timeout time has passed. What is a bit confusing to me is why UnblockClient has a different behaviour when using RedisModule_BlockClientOnKeys than when used with RM_BlockClient.

[guybe7]

the reason is that under normal circumstances a client that was blocked with BlockOnKeys should be released with SignalKeyAsReady and not by UnblockClient.

when you use BlockClient you don't provide privdata; when you unblock it you do (this is usually the case for threads: the worker thread has some information that should be in the reply of the unblocked client).

when you use BlockClientOnKeys you provide a privdata (usually has to do with the target state of the key); when you unblock it (hopefully with SignalKeyAsReady) you don't provide privdata.

using UnblockClient for a client blocked on keys is undesired and the only reason it exists is so that the module can do a proper cleanup in case it is unloaded etc.

i know it's a bit confusing but just remember the pairs BlockClient/UnblockClient and BlockClientOnKeys/SignalKeyAsReady