Firstly, check whether the cacheMap has cached objects to prevent multiple threads from entering the computeIfAbsent method when the Cache corresponding to the name exists under high concurrency, thereby reducing the probability of blocking and deadlocks.
Fixed: https://github.com/spring-projects/spring-framework/issues/30066
Comment From: pivotal-cla
@besscroft Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
Comment From: pivotal-cla
@besscroft Thank you for signing the Contributor License Agreement!
Comment From: bclozel
Could you explain the rationale behind this change? How is this solving the problem listed in #30066?
The ConcurrentHashMap Javadoc states:
A hash table supporting full concurrency of retrievals and high expected concurrency for updates. This class obeys the same functional specification as Hashtable, and includes versions of methods corresponding to each method of Hashtable. > However, even though all operations are thread-safe, retrieval operations do not entail locking, and there is not any support for locking the entire table in a way that prevents all access.
The Javadoc for ConcurrentHashMap#computeIfAbsent states:
Some attempted update operations on this map by other threads may be blocked while computation is in progress, so the computation should be short and simple.
So if any locking is involved, it's because of concurrent updates on the map, not reads. I don't this this change would solve the problem, I think it will only add an extra read operation in case the cache is missing in the map, hurting the performance in general.
I'm declining this PR as a result. We can of course reopen it if you can provide information that backs this change. Maybe a JMH benchmark showing the improvement? Thanks!
Comment From: bclozel
Apologies for the previous comment - after running more tests with the sample provided in #30066 it looks like computeIfAsbent does involve more lock contention, even if all cache entries already exist and we're only performing read operations. This can be reproduced without Spring being involved. I'm still declining this PR as we can improve the implementation more by avoiding null entries in the cache altogether.