[fix] Fix possible potential thread safe bugs #2021
Conversation
| cacheTime[0] = System.currentTimeMillis(); | ||
| timeoutMap.put(key, cacheTime); | ||
| } | ||
| C value = cacheMap.compute(key, (k, v) -> { |
There was a problem hiding this comment.
LGTM, please add a unit test.
There was a problem hiding this comment.
Thanks, I have added unit tests. Please review.
|
I think it is possible to use a global lock to solve this problem, the amount of concurrent addnode removenode will not greatly affect performance. |
How about divides the existing |
| class CommonCacheTest { | ||
|
|
||
| @Mock | ||
| private AbstractConnection<?> mockConnection; |
There was a problem hiding this comment.
ConnectionCommonCache does not depend on external services, so there is no need to use mock.
Here you can create an anonymous AbstractConnection class for testing.
| C value = cacheMap.get(key); | ||
| if (value == null) { | ||
| log.error("[connection common cache] value is null, remove it, key {}.", key); | ||
| cacheMap.remove(key); |
There was a problem hiding this comment.
cacheMap.remove(key);Why was this line of code deleted?
There was a problem hiding this comment.
Because cacheMap uses ConcurrentLinkedHashMap<K, V>. The implementation of put() method in ConcurrentLinkedHashMap<K, V> checks for null value and throws NullPointerException if value is null. So cache.get(key) == null is true only when the key does not exist in the map.
There was a problem hiding this comment.
Please consider upgrading from CLHM to Caffeine at some opportune time. Thanks!
There was a problem hiding this comment.
Please consider upgrading from CLHM to Caffeine at some opportune time. Thanks!
Thanks, we will upgrade to Caffeine in the near future！
Sorry to reply today, personally think this idea is also very good, you can try it. |
What's changed?
Refactored some code to ensure thread safety. And I think
addNode()andremoveNode()inConsistentHashclass may cause thread unsafe issues. Please give some suggestions.Checklist
Add or update API