ConcurrentHashMap as cache

Question

if a ConcurrentHashMap is used as map I ask myself what is the correct way to achieve thread safety?

In a book I found someting like this:

private ConcurrentHashMap<KEY, VALUE> cache = new ConcurrentHashMap<>();

public V put(KEY key, VALUE value) {
    VALUE ret = cache.get(key);
    if (ret == null) {
        ret = cache.putIfAbsent(key, value);
        if (ret == null) {
            ret = value;
        }
    }
    return ret;
}

Now I ask myself isn't it necessary to make the the get and possible put atomic like this:

public V put(KEY key, VALUE value) {
    synchronized(cache) {
        VALUE ret = cache.get(key);
        if (ret == null) {
            ret = cache.putIfAbsent(key, value);
            if (ret == null) {
                ret = value;
            }
        }
    }
    return ret;
}

Because when cache.get() returns null, another thread could invalidate the cache.get() result for the 1st thread?

Cheers Oliver

Answer 1

It is not necessary.

It is true that following code would not be thread-safe as a result of cache.get() can be invalidate by another thread.

VALUE ret = cache.get(key);
if (ret == null) {...}

However, the code is there just for an optimization (atomic operations are more expensive). Atomicity is ensured by map.putIfAbsent() which is atomic and therefore thread-safe. Nevertheless, if cache.get() returns something else then null, expensive atomic operation does not perform.