search

arhs / spring-cache-mongodb

Spring Cache implementation based on MongoDB
MIT License
17 stars 14 forks source link

lock using the cache key instead of a global lock #3

Closed danomatic closed 6 years ago

danomatic commented 7 years ago

Lock using the cache key instead of a global lock, which allows more concurrency without race conditions.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 94.258% when pulling dda06c7cd70889f44f678808db48e066caf62e5e on danomatic:key-based-lock into 99ea324daef9e9c65fddeb8a024f1dcd0c265199 on arhs:master.

dnlprplt commented 7 years ago

Can you point to any evidence that String.intern() can be used for locking?

There are lots of contradicting posts on the subject. Here for example is stated that it's a bad practice: https://stackoverflow.com/questions/133988/problem-with-synchronizing-on-string-objects

Other suggest using a mutex, but wether this is worth it is debatable.

danomatic commented 7 years ago

My rough understanding is that it is safe and efficient, especially for Java 7 and 8. Some details here: http://java-performance.info/string-intern-in-java-6-7-8/

But don't take my word for it. Synchronizing all cache access across all threads was bad for our application, especially because we have slow API calls inside the @Cacheable method.

dnlprplt commented 7 years ago

I've been investigating a little and apart from the article you reference, I haven't found any other evidence that suggests we can safely lock on String.intern. Also the article is not explicitly stating we can lock on it.

I've found another solution though that I think can suit our need. It has been suggested (here https://stackoverflow.com/questions/11124539/how-to-acquire-a-lock-by-a-key) to use the Striped class from Guava. The javadoc on the class explains it all: https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/Striped.java

If you rewrite the code to use the Striped class, I will accept the PR.

danomatic commented 7 years ago

seems too bad to pull in all of guava for that, but it does look nice.

danomatic commented 7 years ago

What about this lighter solution from the same SO post? It seems very similar. https://stackoverflow.com/a/36054928/2020766

dnlprplt commented 7 years ago

Yes, it is based on the same idea. We can go for it as I also prefer keeping the library light.

We should decide on a default value for the size. And make it configurable.