Can my cache be more thread safe?

I have been working on a cache that can handle multiple reads at the same time, but the write is blocking.

Is this code overkill by using both a ConcurrentHashMap and a ReentrantReadWriteLock? Could I get away with just the lock?

Are there any other optimizations I could make?

```
public class QueryCache implements Serializable {

private final ReentrantReadWriteLock rwl = new ReentrantReadWriteLock();
private final Lock readLock = rwl.readLock();
private final Lock writeLock = rwl.writeLock();

/**
* Enum representations of different cache operations. You can put a new cache operation here.
*/
public enum Operation {
INCREMENT,
OVERWRITE
}

/**
* String names of the operations that can be performed. You can add more here.
*/
public static final String INCREMENT = "increment";
public static final String OVERWRITE = "overwrite";

/**
* Contains the key (query) and operations to perform on query
*/
protected final Map> cache;

/**
* Create a new {@link QueryCache} with the initial capacity {@code size}
*
* @param size the initial capacity of the cache
*/
public QueryCache(final int size) {
cache = new ConcurrentHashMap<>(size);
}

/**
* Returns an unmodifiable read-only version of the cache.
* Clears the original cache.
*
* @return an unmodifiable read-only version of the cache.
*/
public Map> flush() {
writeLock.lock();
try {
Map> immutableCache = Collections.unmodifiableMap(new HashMap<>(cache));
cache.clear();
return immutableCache;
} finally {
writeLock.unlock();
}
}

/**
* Returns the size of the cache
*
* @return the size of the cache
*/
public int size() {
readLock.lock();
try {
return cache.size();
} finally {
readLock.unlock();

Concurrent code is very subtle. Even if your code works without locks, removing them could lead to bugs down the road if someone changes the code.

That said, you could remove the locks if you change how ensureCacheKey works. For example, you could remove the locking in that method and call cache.putIfAbsent instead of cache.put. This approach is attractive because the cache.get() check eliminates most unnecessary cacheOperations creations and the few that slip through should be very cheap. Alternately you could have a lock specific to that method (or with more effort, a separate lock for each particular query key).

The usages of the read lock are unnecessary because ConcurrentHashMap supports reads concurrent with writes and because read operations only perform one map operation. The only other place where the cache is modified is when it is cleared. As you may define your concurrency semantics to order chronologically overlapping clear and put operations as you please, and because each write operation only performs one map write, these operations don't need to share a lock.

Some additional comments:

-
In flush(), you are clearing the cache and thus clearing immutableCache. That probably isn't what you want. I suggest replacing cache.clear() with cache = new ... and making the cache variable volatile. In this case make sure that any methods that perform multiple operations on a cache read the cache into a local variable and do all operations on the local variable so they are operating entirely on the same cache.

-
If overwrites and increments never occur on the same keys, your code might be cleaner if you had separate maps for inserts and overwrites.

-
Instead of having strings "increment" and "overwrite" defined as separate top-level variables, make them fields of Operation or lowercase the Operation instances and use Operation.xxx.name().

-
Use !cache.containsKey(x) instead of cache.get(x) == null.

StackExchange Code Review Q#122579, answer score: 4