Enhance thread-safety in ClientSideCaching key retrieval #3268
Conversation
Added per-key locking mechanism to ensure that only a single thread fetches a value from Redis cache for a specific key. This prevents redundant Redis server calls under high concurrent load.
yybmion
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR enhances thread-safety in ClientSideCaching key retrieval by introducing a per-key locking mechanism to avoid redundant Redis calls during high concurrency.
- Added per-key locking using ReentrantLock in caching methods.
- Enhanced integration tests to verify single value loader invocation and proper lock lifecycle management.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/test/java/io/lettuce/core/support/caching/ClientsideCachingIntegrationTests.java | Added tests for concurrent value loading and lock cleanup. |
| src/main/java/io/lettuce/core/support/caching/ClientSideCaching.java | Introduced per-key locking in get methods and updated lock cleanup on invalidation and close. |
Comments suppressed due to low confidence (1)
src/test/java/io/lettuce/core/support/caching/ClientsideCachingIntegrationTests.java:337
- The use of Thread.sleep(200) in the locksShouldBeProperlyCleanedUp test may lead to intermittent failures on slower environments. Consider using a more deterministic waiting mechanism (e.g., Awaitility or additional synchronization constructs) to reliably detect when locks are cleaned up.
Thread.sleep(200);
| value = redisCache.get(key); | ||
|
|
||
| if (value == null) { | ||
| ReentrantLock keyLock = keyLocks.computeIfAbsent(key, k -> new ReentrantLock()); |
There was a problem hiding this comment.
[nitpick] Similar locking logic is used in both get methods. Consider refactoring the common per-key locking mechanism into a helper method to reduce code duplication and improve maintainability.
Copilot uses AI. Check for mistakes.
|
Hey @yybmion could you please format your changes using |
Extract duplicate locking mechanism into a helper method to reduce code duplication and improve maintainability as suggested in the review. Also applied formatter to modified files only.
|
Hi @tishun. Thank you for pointing out the formatting issues - I was having trouble with CRLF/LF line endings. |
Issue
Closes #2402
The current implementation of the get method in the ClientSideCaching class does not provide a mechanism to ensure exclusive access by a single thread when retrieving a value from the Redis server for a specific key. As a result, in high concurrent load scenarios involving the same key, the current implementation may cause redundant and unnecessary calls to the Redis server.
Solution
Added a per-key locking mechanism using
ReentrantLockto ensure that only a single thread can fetch a value from Redis for a specific key at any given time.Tests
valueLoaderShouldBeInvokedOnceForConcurrentRequests: Verifies that when multiple threads concurrently access the same key using a valueLoader, the loader is called exactly oncelocksShouldBeProperlyCleanedUp: Verifies the proper lifecycle management of locks (creation, cleanup on invalidation, recreation, and final cleanup)