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 <wangyuancode@163.com>
Co-authored-by: debing.sun <debing.sun@redis.com>
This commit is contained in:
Mincho Paskalev 2025-07-02 11:55:57 +03:00 committed by GitHub
parent 0d8e750883
commit ad8c7de3fe
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 28 additions and 1 deletions

View File

@ -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);

View File

@ -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]