Can my cache be more thread safe?

Question

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<QueryKey, Map<Operation, BasicDBObject>> 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<QueryKey, Map<Operation, BasicDBObject>> flush() {
        writeLock.lock();
        try {
            Map<QueryKey, Map<Operation, BasicDBObject>> 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();
        }
    }


    /**
     * Clears the cache
     */
    public void clear() {
        writeLock.lock();
        try {
            cache.clear();
        } finally {
            writeLock.unlock();
        }
    }


    /**
     * General insertion call that will pass the request to the inserter with the matching {@code insertionMode} .
     * Will return if a matching insert operation was found.
     *
     * @param insertionMode the name of the insertion to perform
     * @param key           the current query
     * @param fieldName     the name of the field to cache
     * @param fieldValue    the value of the field to cache.
     * @return if a matching insert operation was found.
     */
    public boolean handleInsertion(final String insertionMode, final QueryKey key, final String fieldName, final Object fieldValue) {
        switch (insertionMode) {
            case INCREMENT:
                ensureCacheKey(key);
                insertIncrement(key, fieldName, getFieldValue(fieldValue));
                return true;
            case OVERWRITE:
                ensureCacheKey(key);
                insertOverwrite(key, fieldName, getFieldValue(fieldValue));
                return true;
            default:
                return false;
        }
    }


    protected static Object getFieldValue(final Object field) {
        if (field instanceof JsonNode) {
            JsonNode node = (JsonNode) field;
            if (node.isInt()) return node.asInt();
            if (node.isLong()) return node.asLong();
            if (node.isBoolean()) return node.asBoolean();
            return node.asText();
        }
        return field;
    }


    /**
     * Ensures that there is an empty, non-null value, for each available operation for the provided {@code key}.
     *
     * @param key
     */
    private void ensureCacheKey(final QueryKey key) {
        if (null == cache.get(key)) {
            writeLock.lock();
            try {
                if (null == cache.get(key)) {
                    Map<Operation, BasicDBObject> cacheOperations = new ConcurrentHashMap<>();
                    for (Operation operation : Operation.values()) {
                        cacheOperations.put(operation, new BasicDBObject());
                    }
                    cache.put(key, cacheOperations);
                }

            } finally {
                writeLock.unlock();
            }
        }
    }


    /**
     * Increments any existing increment query in the {@code incrementCache} or inserts it.
     *
     * @param key        the current query
     * @param fieldName  the name of the field to cache
     * @param fieldValue the value of the field to cache
     */
    private void insertIncrement(final QueryKey key, final String fieldName, final Object fieldValue) {

        Long value;
        if (fieldValue instanceof Integer) {
            value = ((Integer) fieldValue).longValue();
        } else {
            value = (Long) fieldValue;
        }

        cache.get(key).get(Operation.INCREMENT).putIfAbsent(fieldName, new AtomicLong());
        ((AtomicLong) cache.get(key).get(Operation.INCREMENT).get(fieldName)).addAndGet(value);
    }


    /**
     * Overwrites any existing overwrite query in the {@code overwriteCache} or inserts it.
     *
     * @param key        the current query
     * @param fieldName  the name of the field to cache
     * @param fieldValue the value of the field to cache
     */
    private void insertOverwrite(final QueryKey key, final String fieldName, final Object fieldValue) {
        writeLock.lock();
        try {
            cache.get(key).get(Operation.OVERWRITE).put(fieldName, fieldValue);
        } finally {
            writeLock.unlock();
        }
    }
}

Answer 1

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:

1. 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.

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

3. 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().

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