Fix global cache - #9
Open
chejinge wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughEviction was refactored to track multiple redisDb instances globally. Eviction pool entries now record source DBs, eviction routines iterate over a global DB list to populate/select candidates, and RcCreateCacheHandle registers new DBs in that global list. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RcCreateCacheHandle as RcCreateCacheHandle()
participant GlobalDBList as Global DB List (g_all_redis_dbs)
participant MemoryCheck as freeMemoryIfNeeded()
participant EvictionPool as evictionPoolPopulate()
participant TargetDB as Target DB (from pool)
Client->>RcCreateCacheHandle: create cache handle
RcCreateCacheHandle->>GlobalDBList: lock g_db_list_mutex / register new DB
GlobalDBList-->>RcCreateCacheHandle: unlock / return handle
Client->>MemoryCheck: freeMemoryIfNeeded(trigger_db)
MemoryCheck->>GlobalDBList: lock g_db_list_mutex
MemoryCheck->>GlobalDBList: iterate g_all_redis_dbs
loop per registered DB
MemoryCheck->>EvictionPool: evictionPoolPopulate(sampledict, keydict, pool, db)
Note over EvictionPool: pool entries store .db -> source DB
end
MemoryCheck->>MemoryCheck: select bestkey & bestdb from pool
MemoryCheck->>TargetDB: delete bestkey from bestdb
TargetDB-->>MemoryCheck: deletion result
MemoryCheck-->>Client: return eviction outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db.c`:
- Around line 457-466: The loop currently picks the first non-empty DB and
samples a random key only from it, biasing evictions; instead, under
MAXMEMORY_ALLKEYS_RANDOM or volatile-random policy compute each DB's eligible
key count using dictSize(curr_db->dict) or dictSize(curr_db->expires) (based on
maxmemory_policy), sum them to total, pick a single uniform random index in
[0,total-1], iterate the DBs subtracting each eligible size until the random
index falls into a DB, then call dictGetRandomKey on that chosen DB and set
bestdb/bestkey/de accordingly; keep the existing pthread_mutex_lock/unlock and
handle the total==0 case by returning/continuing as before.
- Around line 422-430: The code unlocks g_db_list_mutex before using/deleting
the selected redisDb (bestdb), allowing RcDestroyCacheHandle() to free it
concurrently; fix by ensuring the DB is pinned for the entire select+delete
sequence: after reading a redisDb* from g_all_redis_dbs (inside the
g_db_list_mutex) call the appropriate pin/retain API (so it cannot be
destroyed—e.g., a RcHold/retain function corresponding to RcDestroyCacheHandle)
or otherwise increment its reference count, then unlock the mutex and perform
evictionPoolPopulate/select+delete, and finally unpin/release the DB;
alternatively, keep g_db_list_mutex held across the full lifetime-sensitive
section (for functions evictionPoolPopulate and any code that dereferences
bestdb) so the DB cannot be freed while in use. Ensure to apply the same change
to the other blocks mentioned (lines analogous to 435-447, 457-468, 471-475)
referencing g_all_redis_dbs, g_db_list_mutex, evictionPoolPopulate, bestdb and
RcDestroyCacheHandle.
In `@redis.c`:
- Around line 71-79: RcCreateCacheHandle() registers every new redisDb* in the
global registry (g_all_redis_dbs/g_redis_db_num) but RcDestroyCacheHandle() only
frees the db, leaving stale pointers and causing use-after-free during registry
iteration; modify RcDestroyCacheHandle() to, under the g_db_list_mutex, find the
matching redisDb* in g_all_redis_dbs, remove it (either shift subsequent entries
down or replace it with the last entry), decrement g_redis_db_num, clear the
removed slot (set NULL if applicable), then unlock before freeing the db; ensure
you handle the case where the pointer isn’t found gracefully.
In `@t_hash.c`:
- Line 13: Remove the stdout debug printing from genericHgetall: delete the
printf/puts calls that print each hash field/value (the prints around lines
~599-606 in function genericHgetall) and remove the `#include` "stdio.h" at the
top (line 13) since it was only pulled in for those debug prints; if you still
need diagnostics, replace them with the project's non-blocking logging facility
rather than stdout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bafff50-1829-4c89-8b62-1cea1b08355c
📒 Files selected for processing (5)
db.cevict.cevict.hredis.ct_hash.c
Comment on lines
+422
to
+430
| pthread_mutex_lock(&g_db_list_mutex); | ||
| for (int i = 0; i < g_redis_db_num; i++) { | ||
| redisDb *curr_db = (redisDb *)g_all_redis_dbs[i]; | ||
| dict *d = (maxmemory_policy & MAXMEMORY_FLAG_ALLKEYS) ? curr_db->dict : curr_db->expires; | ||
| if (dictSize(d) != 0) { | ||
| evictionPoolPopulate(d, curr_db->dict, pool, curr_db); | ||
| } | ||
| } | ||
| if (!keys) break; /* No keys to evict. */ | ||
| pthread_mutex_unlock(&g_db_list_mutex); |
There was a problem hiding this comment.
The registry mutex does not protect bestdb long enough.
Both eviction paths read redisDb * values from g_all_redis_dbs while holding g_db_list_mutex, then dereference and delete through bestdb after unlocking. A concurrent RcDestroyCacheHandle() can free that DB in between, so this can walk or delete through a dangling pointer. The DB needs to stay pinned for the full select+delete sequence, or the lock needs to cover the whole lifetime-sensitive section.
Also applies to: 435-447, 457-468, 471-475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db.c` around lines 422 - 430, The code unlocks g_db_list_mutex before
using/deleting the selected redisDb (bestdb), allowing RcDestroyCacheHandle() to
free it concurrently; fix by ensuring the DB is pinned for the entire
select+delete sequence: after reading a redisDb* from g_all_redis_dbs (inside
the g_db_list_mutex) call the appropriate pin/retain API (so it cannot be
destroyed—e.g., a RcHold/retain function corresponding to RcDestroyCacheHandle)
or otherwise increment its reference count, then unlock the mutex and perform
evictionPoolPopulate/select+delete, and finally unpin/release the DB;
alternatively, keep g_db_list_mutex held across the full lifetime-sensitive
section (for functions evictionPoolPopulate and any code that dereferences
bestdb) so the DB cannot be freed while in use. Ensure to apply the same change
to the other blocks mentioned (lines analogous to 435-447, 457-468, 471-475)
referencing g_all_redis_dbs, g_db_list_mutex, evictionPoolPopulate, bestdb and
RcDestroyCacheHandle.
Comment on lines
+457
to
+466
| pthread_mutex_lock(&g_db_list_mutex); | ||
| for (int i = 0; i < g_redis_db_num; i++) { | ||
| redisDb *curr_db = (redisDb *)g_all_redis_dbs[i]; | ||
| dict *d = (maxmemory_policy == MAXMEMORY_ALLKEYS_RANDOM) ? curr_db->dict : curr_db->expires; | ||
| if (dictSize(d) != 0) { | ||
| de = dictGetRandomKey(d); | ||
| bestkey = dictGetKey(de); | ||
| bestdb = curr_db; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Random eviction is now biased toward the first non-empty DB.
This loop stops at the first eligible DB and then samples a random key only inside that DB. With multiple registered DBs, lower-index DBs will absorb nearly all allkeys-random / volatile-random evictions, which is not random over the global keyspace. Pick the DB proportionally to its eligible key count, or sample from the combined candidate set before deleting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db.c` around lines 457 - 466, The loop currently picks the first non-empty DB
and samples a random key only from it, biasing evictions; instead, under
MAXMEMORY_ALLKEYS_RANDOM or volatile-random policy compute each DB's eligible
key count using dictSize(curr_db->dict) or dictSize(curr_db->expires) (based on
maxmemory_policy), sum them to total, pick a single uniform random index in
[0,total-1], iterate the DBs subtracting each eligible size until the random
index falls into a DB, then call dictGetRandomKey on that chosen DB and set
bestdb/bestkey/de accordingly; keep the existing pthread_mutex_lock/unlock and
handle the total==0 case by returning/continuing as before.
Comment on lines
+71
to
+79
| redisDb *db = createRedisDb(); | ||
| if (db) { | ||
| pthread_mutex_lock(&g_db_list_mutex); | ||
| if (g_redis_db_num < MAX_CACHE_DB_NUM) { | ||
| g_all_redis_dbs[g_redis_db_num++] = db; | ||
| } | ||
| pthread_mutex_unlock(&g_db_list_mutex); | ||
| } | ||
| return db; |
There was a problem hiding this comment.
Unregister destroyed DBs from the global registry.
RcCreateCacheHandle() now publishes every redisDb *, but RcDestroyCacheHandle() still just frees it. That leaves freed pointers in g_all_redis_dbs, keeps g_redis_db_num growing forever, and eventually returns live handles that are never registered once the array fills. Since db.c now iterates this registry during eviction, stale entries turn into a use-after-free path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@redis.c` around lines 71 - 79, RcCreateCacheHandle() registers every new
redisDb* in the global registry (g_all_redis_dbs/g_redis_db_num) but
RcDestroyCacheHandle() only frees the db, leaving stale pointers and causing
use-after-free during registry iteration; modify RcDestroyCacheHandle() to,
under the g_db_list_mutex, find the matching redisDb* in g_all_redis_dbs, remove
it (either shift subsequent entries down or replace it with the last entry),
decrement g_redis_db_num, clear the removed slot (set NULL if applicable), then
unlock before freeing the db; ensure you handle the case where the pointer isn’t
found gracefully.
| #include "db.h" | ||
| #include "ziplist.h" | ||
| #include "util.h" | ||
| #include "stdio.h" |
There was a problem hiding this comment.
Remove stdout debug logging from genericHgetall before merge.
Line 601 and Line 605 print every hash field/value to stdout. This introduces data-leak risk and adds blocking I/O in a frequently used path. Line 13 is only needed for this debug output.
Proposed fix
-#include "stdio.h"
@@
if (flags & OBJ_HASH_KEY) {
addHashIteratorCursorToReply(hi, OBJ_HASH_KEY, &((*items+i)->field));
- printf("field: %s\n", (*items+i)->field);
}
if (flags & OBJ_HASH_VALUE) {
addHashIteratorCursorToReply(hi, OBJ_HASH_VALUE, &((*items+i)->value));
- printf("value: %s\n", (*items+i)->value);
}Also applies to: 599-606
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@t_hash.c` at line 13, Remove the stdout debug printing from genericHgetall:
delete the printf/puts calls that print each hash field/value (the prints around
lines ~599-606 in function genericHgetall) and remove the `#include` "stdio.h" at
the top (line 13) since it was only pulled in for those debug prints; if you
still need diagnostics, replace them with the project's non-blocking logging
facility rather than stdout.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Bug Fixes
Documentation
Chores