Fix cache proxy connection unbalanced command send and read#3908
Fix cache proxy connection unbalanced command send and read#3908tzongw wants to merge 5 commits intoredis:masterfrom
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
|
Hi @tzongw, thank you for your contribution! Can you please check test failures? |
There was a problem hiding this comment.
Pull request overview
This PR fixes race conditions in CacheProxyConnection between send_command and read_response methods by changing from tracking cache keys to tracking cache entry objects. The core improvement is storing a reference to the entire CacheEntry object instead of just the cache key, which enables proper identity checking and prevents unbalanced command send/read operations.
Key changes:
- Renamed
_current_command_cache_keyto_current_command_cache_entryto store full entry references - Added explicit status checks for
CacheEntryStatus.VALIDvsCacheEntryStatus.IN_PROGRESSto handle concurrent cache operations - Implemented identity-based comparison (
is) to detect when cache entries are replaced by other threads
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| redis/connection.py | Core fix: Changed from tracking cache keys to cache entry objects, added explicit cache entry status validation, and implemented identity checks to detect entry replacement during concurrent operations |
| tests/test_connection.py | Updated existing test for variable rename; added three comprehensive test cases covering cache entry in-progress, entry disappearing between send/read, and entry appearing between send/read scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
redis/connection.py
Outdated
| # Check if entry still exists. | ||
| if self._cache.get(cache_key) is not None: |
There was a problem hiding this comment.
The check at line 1479 only verifies that some entry exists in the cache, but doesn't verify it's the same entry object as cache_entry. If the entry was invalidated and a new entry was created by another thread between lines 1477 and 1479, the condition would pass but line 1480 would store a reference to the old invalidated entry. This could lead to using stale cached data. The check should verify the entry identity: if self._cache.get(cache_key) is cache_entry:
| # Check if entry still exists. | |
| if self._cache.get(cache_key) is not None: | |
| # Check if the same entry still exists in the cache. | |
| if self._cache.get(cache_key) is cache_entry: |
@petyaslavova updated |
54ccb16 to
6c76951
Compare
Description of change
Fix
CacheProxyConnectionrace conditions betweensend_commandandread_response, as test cases described, covers issue #3600.Test cases
These cause unbalanced command send and read and the connection will hang forever or read a mismatched response.