.3493622109966576:a197018d5c60a54a51b49ca15d6fb0b3_69e7125e81ef3ff6db540126.69e7462981ef3ff6db540408.69e74628d6025e57c275fa6c:Trae CN.T(2026/4/21 17:40:57)#4042
Conversation
实现线程安全的本地内存缓存，支持 TTL 和 LRU 淘汰策略。当配置 local_cache_ttl 和 local_cache_max_size 时，自动为 GET 命令添加本地缓存层，减少对远端 Redis 的高频读取延迟。同时处理 SET/DEL 命令的缓存同步问题。
调整连接获取时机以避免潜在的内存泄漏 优化本地缓存检查时的连接释放逻辑
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Reviewed by Cursor Bugbot for commit 8d310b6. Configure here.
| command_upper = command_name.upper() | ||
| if command_upper == "GET" and len(args) >= 2: | ||
| key = args[1] | ||
| self._local_cache.set(key, result) |
There was a problem hiding this comment.
Cached None values waste space, never serve hits
High Severity
LocalCache.get() returns None for both cache misses and cached None values, but the caller at _execute_command uses if cached_value is not None to detect cache hits. When Redis GET returns None (key doesn't exist), it's stored in the cache via self._local_cache.set(key, result), but subsequent lookups always treat it as a miss. These None entries consume cache capacity and evict valid entries through LRU, degrading cache effectiveness. A sentinel object distinct from None is needed to differentiate "not in cache" from "cached None."
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8d310b6. Configure here.
| except UnicodeDecodeError: | ||
| command_upper = command_name.decode('latin-1').upper() | ||
| else: | ||
| command_upper = command_name.upper() |
There was a problem hiding this comment.
Redundant command_upper computation after command execution
Low Severity
The command_upper variable is computed identically twice — first at the pre-execution cache lookup block, then again in the post-execution cache store block — under the same self._local_cache is not None guard. Since reaching the second block guarantees the first block already ran (the only early return is line 809, which exits the function entirely), command_upper is already in scope and can be reused directly.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8d310b6. Configure here.
| self._local_cache.delete(key) | ||
| else: | ||
| key = args[1] | ||
| self._local_cache.delete(key) |
There was a problem hiding this comment.
Many write commands don't invalidate the local cache
High Severity
Cache invalidation only covers SET, SETEX, and DEL, but many other write commands (INCR, DECRBY, APPEND, GETSET, GETDEL, MSET, SETRANGE, INCRBYFLOAT, etc.) all flow through execute_command and modify key values without invalidating the local cache. A sequence like r.get("counter") followed by r.incr("counter") then r.get("counter") will silently return the stale cached value, producing incorrect results with no indication of error.
Reviewed by Cursor Bugbot for commit 8d310b6. Configure here.
|
Hi @zilvya , thank you for your contribution! Could you please update the PR title and description to be in English, and also replace the non-English comments in the code as well as the commit messages, so they’re clear to the broader community? Additionally, could you share more context on the motivation behind this feature? We already have client-side caching implemented in the synchronous standalone client, so it would be helpful to understand the use case this PR is addressing and how it differs or extends the existing functionality. |
2 participants
Description of change
Please provide a description of the change here.
Pull Request check-list
Please make sure to review and check all of these items:
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Note
Medium Risk
Adds a new local in-process caching layer in the core command execution path, which can change read-after-write behavior and introduce staleness/consistency edge cases if invalidation is incomplete. Also touches connection release logic, though the change is small and guarded.
Overview
Adds an optional, experimental local in-process cache to
Redisthat cachesGETresults with configurable TTL and max size (LRU eviction) via newlocal_cache_ttl/local_cache_max_sizeconstructor args.Updates command execution to short-circuit
GETfrom the local cache when enabled, populate the cache after successfulGET, and invalidate cached keys onSET/SETEX/DEL; also tightens connection-pool release in_execute_commandto only release when aconnwas acquired.Reviewed by Cursor Bugbot for commit 8d310b6. Bugbot is set up for automated code reviews on this repo. Configure here.