From ad8c7de3fe563ff6ccdce7d939c0d50e2a8f19fa Mon Sep 17 00:00:00 2001 From: Mincho Paskalev Date: Wed, 2 Jul 2025 11:55:57 +0300 Subject: [PATCH] Fix assertion in updateClientMemUsageAndBucket (#14152) ## Description `updateClientMemUsageAndBucket` is called from the main thread to update memory usage and memory bucket of a client. That's why it has assertion that it's being called by the main thread. But it may also be called from a thread spawned by a module. Specifically, when a module calls `RedisModule_Call` which in turn calls `call`->`replicationFeedMonitors`->`updateClientMemUsageAndBucket`. This is generally safe as module calls inside a spawned thread should be guarded by a call to `ThreadSafeContextLock`, i.e the module is holding the GIL at this point. This commit fixes the assertion inside `updateClientMemUsageAndBucket` so that it encompasses that case also. Generally calls from module-spawned threads are safe to operate on clients that are not running on IO-threads when the module is holding the GIL. --------- Co-authored-by: Yuan Wang Co-authored-by: debing.sun --- src/server.c | 11 ++++++++++- tests/unit/moduleapi/usercall.tcl | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/server.c b/src/server.c index 0728707d3..f1ba5555c 100644 --- a/src/server.c +++ b/src/server.c @@ -1055,7 +1055,16 @@ void removeClientFromMemUsageBucket(client *c, int allow_eviction) { * returns 1 if client eviction for this client is allowed, 0 otherwise. */ int updateClientMemUsageAndBucket(client *c) { - serverAssert(pthread_equal(pthread_self(), server.main_thread_id) && c->conn); + /* The unlikely case this function was called from a thread different + * than the main one is a module call from a spawned thread. This is safe + * since this call must have been made after calling + * RedisModule_ThreadSafeContextLock i.e the module is holding the GIL. In + * that special case we assert that at least the updated client's + * running_tid is the main thread. The true main thread is allowed to call + * this function on clients handled by IO-threads as it makes sure the + * IO-threads are paused, f.e see cleintsCron() and evictClients(). */ + serverAssert((pthread_equal(pthread_self(), server.main_thread_id) || + c->running_tid == IOTHREAD_MAIN_THREAD_ID) && c->conn); int allow_eviction = clientEvictionAllowed(c); removeClientFromMemUsageBucket(c, allow_eviction); diff --git a/tests/unit/moduleapi/usercall.tcl b/tests/unit/moduleapi/usercall.tcl index 0f7a318f7..05f333180 100644 --- a/tests/unit/moduleapi/usercall.tcl +++ b/tests/unit/moduleapi/usercall.tcl @@ -134,6 +134,24 @@ start_server {tags {"modules usercall"}} { assert_match {*cmd=usercall.call_with_user_flag*} [dict get $entry client-info] } + test {server not crashing when MONITOR is fed from spawned thread} { + set rd [redis_deferring_client] + $rd monitor + + r ACL LOG RESET + assert_equal [r usercall.reset_user] OK + assert_equal [r usercall.add_to_acl "~* &* +@all -set"] OK + + r flushdb + r set x x + + # This is enough. This checks that we don't crash inside + # updateClientMemUsageAndBucket + assert_equal x [r usercall.call_with_user_bg C get x] + + $rd close + } + start_server {tags {"wait aof network external:skip"}} { set slave [srv 0 client] set slave_host [srv 0 host]