RedisCache.get(Object, Callable) synchronizes on entire cache instead of individual keys

[Traubenfuchs]

The following code can be found in org.springframework.data.redis.cache.RedisCache.

	private final Lock lock = new ReentrantLock();

	@Override
	@SuppressWarnings("unchecked")
	public <T> T get(Object key, Callable<T> valueLoader) {

		ValueWrapper result = get(key);

		return result != null ? (T) result.get() : getSynchronized(key, valueLoader);
	}

	@Nullable
	@SuppressWarnings("unchecked")
	private <T> T getSynchronized(Object key, Callable<T> valueLoader) {

		lock.lock();

		try {
			ValueWrapper result = get(key);
			return result != null ? (T) result.get() : loadCacheValue(key, valueLoader);
		} finally {
			lock.unlock();
		}
	}

I believe there could be a significant amount of unnecessary locking happening here.

I don't think that this method should be locked via an instance-global ReentrantLock instance.

Instead, it should be locked on a per key basis, as follows:

        private final ConcurrentHashMap<Object, Lock> keyToLock = new ConcurrentHashMap<>();

	@Nullable
	@SuppressWarnings("unchecked")
	private <T> T getSynchronized(Object key, Callable<T> valueLoader) {

                Lock lock = keyToLock.computeIfAbsent(key, k -> new ReentrantLock());

		lock.lock();

		try {
			ValueWrapper result = get(key);
			return result != null ? (T) result.get() : loadCacheValue(key, valueLoader);
		} finally {
			lock.unlock();
		}
	}

Plus additional cleanup calls to the ConcurrentHashMap for clear and evict calls to RedisCache.

What do you think?

[mp911de]

You're right that we lock the entire cache instead of individual keys for value-loading. Back when we implemented this functionality we considered lock-management on a per-key basis a pretty significant overhead.

Cache-locking happens inside RedisCacheWriter. Alternatively, we could reuse RedisCacheWriter's lock and have a lock for the global cache that would also affect other instances of RedisCache on other nodes.

This requires a bit more thought on how refining locking would fit into the bigger picture.