Race Condition in JCacheManager between createCache() and getCache()

[Yogu]

As far as I understand, jsr-107's CacheManager is supposed to be thread-safe, and it is only practical for it to be.

There is a possibility for the cache configuration created by JCacheManager.createCache() to be configured incorrectly if JCacheManager.getCache() is called at the same time with the same cache name. In particular, there is a realistic chance that the statistics and management beans are not created.

This works as follows:

• createCache is called.
• createCache makes sure allCaches does not include the cache
• createCache adds the cache to the ehCache-backed cacheManager
• Now, getCache(String, Class, Class) is called with the same argument.
• getCache tries to find the same cache in allCaches but does it does not exist
• getCache looks in the ehCache-backed cacheManager, and finds the cache as it's already added there
• getCache creates a new JCache based on a JCacheConfiguration with the default configuration and puts it into allCaches
• createCache creates a new JCache with the correct configuration and tries to add it with allCaches.putIfAbsent, but it already exists
• createCache returns the misconfigured cache that is already present in allCaches, and does not call enableStatistics or enableManagement for it.

The same problem occurs when using the second overload, getCache(String):

• createCache is called.
• createCache makes sure allCaches does not include the cache
• createCache adds the cache to the ehCache-backed cacheManager
• getCache(String) is called with the same cache name as argument.
• getCache looks in allCaches, but does not find the cache
• getCache calls refreshAllCaches()
• refreshAllCaches iterates over all caches in the ehCache-backed cacheManager, creates new JCache instances for them and puts them into allCaches
• The problem is that new JCacheConfiguration() does not properly set the managementEnabled and statisticsEnabled attributes when a ehCache CacheConfiguration is provided as argument
• createCache again uses the already existing, but misconfigured cache and returns that, enableStatistics and enableManagement are not called.

I see three possibilities to solve this problem:

• Use synchronized on all the methods touching or reading allCaches. This may lead to performance loss, but is the simplest to implement
• Use ReadWriteLock so that concurrent e.g. getCache or getCacheNames can be called concurrently.
• Lock cache names right when they are created using a concurrent set, and do not synchronize ehcaches to allCaches for those in this set. However, I don't think this will work because getCache is expected to wait for a cache being added to be completely configured befure returning it. That's only possible with locking.

I will create a pull request to fix this when we have agreed on a solution.

[alexsnaps]

Hey Jan,
Sorry for getting back to you only now... somehow I managed to miss a bunch of github notifications over the last week.

Now on the issue itself: There is indeed an issue there. There is even two afaict:

• The race you mentioned
• The return on line 115, where it's not the validation that's missing, but this should just plain throw a CacheException as per spec.

Now, solving these two can I think be a single changeset. I'd propose a fourth alternative: How about we change the allCaches Map to Map<String, Future<JCache>> ? So that, in createCache we first always try to install a new Future with putIfAbsent, if that fails, we throw (the cache existed already).

If it succeeds, that thread is now responsible for installing the actual JCache instance. Other concurrent threads would hit the Future and possibly block on .get() until that other thread installed it.

I think this minimizes the contention, while solving the race. How does that sound to you?

[Yogu]

Thanks for your reply. I did my best implementing the fix with Futures as you suggested. However, I'm not quite sure if it is completely correct, especially the error handling. Please have a look at #42.

[alexsnaps]

@Yogu Thanks a lot for the PR! I'll have a look... sadly probably not today anymore, but I'll put this on my todo list for tomorrow!

[fossf]

@Yogu can you please sign a contributor agreement for acceptance of your fix?
https://confluence.terracotta.org/display/release/How+to+Become+a+Terracotta+Contributor
Thank you

[Yogu]

Sorry for the delay, I had to check some things first. I mailed the signed agreement to [email protected].