SimpleCacheManager should not synchronize on AbstractCacheManager#cacheMap

[mpretzer]

Affects: 5.1.8

By inheriting AbstractCacheManager#getCache, SimpleCacheManager synchronizes on cacheMap, when the requested Cache is not available.

Since SimpleCacheManager uses predefined caches, this creates superfluous synchronizations.

The same is actually true for RedisCacheManager, when inflight Cache allocation is disabled.

This hits us on every request, since we are using CompositeCacheManager, which first checks our SimpleCacheManager holding only select caches and only afterwards to the manager holding the actual cache for certain requests.

[mpretzer]

I'm not sure about the fix. Won't this lead to getMissingCache being potentially called multiple times which might be a problem when cache creation is expensive?

My approach would have been to introduce the possibility for subclasses to signal whether in-flight cache creation is supported and only then enter the synchronized block (like RedisCacheManager has the allowInFlightCacheCreation flag). I know this would require potential changes in downstream projects but maybe a default is ok for most implementations?

//AbstractCacheManager
protected boolean allowInFlightCacheCreation() {
  return true;
}

//SimpleCacheManager
protected boolean allowInFlightCacheCreation() {
  return false;
}

[jhoeller]

I was wondering about that myself, but looking at the actual implementations of getMissingCache, they all just create thin org.springframework.cache.Cache wrappers for some underlying provider facilities. In the case of JCache and EhCache, the actual cache region has been created by the cache provider in the meantime and is only being retrieved and wrapped on the fly there. In the case of Redis, it's a thin wrapper as well. Enforcing high-level synchronization for that purpose seems like a bad tradeoff if all we're preventing is multiple wrapper instances in a race condition scenario, which is why I eventually went with a non-synchronized getMissingCache call where only the registration of a returned wrapper is synchronized eventually, making sure that a consistent wrapper handle is being exposed to the application.

If a specific getMissingCache implementation turns out to be actually expensive for potential concurrent calls with the same cache name, a local lock or provider-level lock can still be applied in the concrete implementation, potentially even an isolated lock for the cache provider region. In particular in such an expensive scenario, fully locking the entire cache map at AbstractCacheManager level - and therefore blocking any cache access which happens to go into the missing cache path - seems like a particularly bad tradeoff. I'm not aware of any such expensive implementation, but I would argue that concurrency in such scenarios might actually benefit most from this change.