Hello, I have a question on thread safety of Redis Module APIs like RedisModule_GetClientInfoById and guidance on the usage of them. We have a custom redis module which calls this function on two occasions, one from a background timer thread and one from blocked client context (similar to a function threadmain in this example https://redis.io/docs/reference/modules/modules-blocking-ops/#how-blocking-and-resuming-works) which is also a separate thread during some custom command operation.

We have observed crash from this function in rare occasions and seems like when connections getting added or deleted the rax tree for maintaining connection in redis gets changed and that might hamper if some other thread from redis module is calling RedisModule_GetClientInfoById which access same rax tree. I wanted to know what is the best practice to use these functions if module needs to call these functions from a thread.

Is the following code snippet makes it thread safe for all cases(for examaple: the clientId for which it is being called is marked for deletion due to disconnection and module is yet to aware about that)?

// bc would be null from timer thread where it is not for a specific blocked client.
RedisModuleCtx *ctx = RedisModule_GetThreadSafeContext(bc);
RedisModule_ThreadSafeContextLock(ctx);

RedisModule_GetClientInfoById(&clientInfo, clientId);

RedisModule_ThreadSafeContextUnlock(ctx);
RedisModule_FreeThreadSafeContext(ctx);

Comment From: sundb

RedisModule_GetClientInfoById is not thread-safe, clients may be changed in the main thread. Looks like we're still missing some api thread-safety reminders. The example you gave is thread-safe. In fact most of the module api's are not thread-safe, perhaps we should tell exactly what is thread-safe, rather than what is not.

Comment From: samsaha-ms

Thanks for confirmation. Just to clarify one thing, what is the difference between null vs blocked client as argument while calling RedisModule_GetThreadSafeContext() in this scenario? Are both same for RedisModule_GetClientInfoById() as rax tree for connections is global context wrt redis not specific to a connection?

Comment From: sundb

Thanks for confirmation. Just to clarify one thing, what is the difference between null vs blocked client as argument while calling RedisModule_GetThreadSafeContext() in this scenario?

Passing NULL determines whether ctx is associated to a client, and if so you can communicate with the client using such RM_Reply* api.

Are both same for RedisModule_GetClientInfoById() as rax tree for connections is global context wrt redis not specific to a connection?

regardless of the ctx, its behaviors is the same.

Comment From: samsaha-ms

Thanks for the clarification!

Comment From: samsaha-ms

Got my answers!

Comment From: samsaha-ms

Reopened it for documentation triage. once done can be closed.

Comment From: samsaha-ms

Hi @sundb, I had another doubt, does the reply_callbacks or timeout_callbacks are thread safe that we register in following APIs? RedisModuleBlockedClient *RedisModule_BlockClient(RedisModuleCtx *ctx, RedisModuleCmdFunc reply_callback, RedisModuleCmdFunc timeout_callback, void (*free_privdata)(void*), long long timeout_ms); What I mean by thread safe is if I call any thread unsafe module APIs like RedisModule_GetClientInfoById() inside the registered callback handlers do I still need to use lock/unlock mechanism?

Comment From: sundb

@samsaha-ms don't require lock, because we assure that these callbacks can be called by only one thread.

Comment From: samsaha-ms

I see, so these callbacks are run under main thread context one by one, and there is no way client connection rax tree can change in-between right?

Comment From: sundb

@samsaha-ms sorry, i need to retract my reply above. because these callbacks may be called in the main thread or in the module thread, calling these non-thread-safe api also needs locking. as in the module example below: https://github.com/redis/redis/blob/0b34396924eca4edc524469886dc5be6c77ec4ed/tests/modules/blockonbackground.c#L30:L55

Comment From: samsaha-ms

Thanks @sundb for the info!