Replace double checked locking in concurrent environment

Question

I have already topic with same code:

public abstract class Digest {
    private Map<String, byte[]> cache = new HashMap<>();

    public byte[] digest(String input) {
        byte[] result = cache.get(input);
        if (result == null) {
            synchronized (cache) {
                result = cache.get(input);
                if (result == null) {
                    result = doDigest(input);
                    cache.put(input, result);
                }
            }
        }
        return result;
    }

    protected abstract byte[] doDigest(String input);
}

At previous I got prove that code is not thread safe.

At this topic I want to provide solutions which I have in my head and I ask you to review these solutions:

Solution#1 through ReadWriteLock:

public abstract class Digest {

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

    private Map<String, byte[]> cache = new HashMap<>(); // I still don't know should I use volatile or not

    public byte[] digest(String input) {
        byte[] result = null;
        readLock.lock();
        try {
            result = cache.get(input);
        } finally {
            readLock.unlock();
        }
        if (result == null) {
            writeLock.lock();
            try {
                result = cache.get(input);
                if (result == null) {
                    result = doDigest(input);
                    cache.put(input, result);
                }
            } finally {
                writeLock.unlock();
            }
        }

        return result;
    }

    protected abstract byte[] doDigest(String input);
}

Solution#2 through CHM

public abstract class Digest {
    private Map<String, byte[]> cache = new ConcurrentHashMap<>(); //should be volatile?

    public byte[] digest(String input) {
        return cache.computeIfAbsent(input, this::doDigest);
    }

    protected abstract byte[] doDigest(String input);
}

Please review correctness of both solutions. It is not question about what the solution better. I undestand that CHM better. Please, review correctnes of implementation

Answer 1

Unlike the clusterfudge we got into in the last question, this is better.

As was shown in the prefious question's duplicate, the original code is not thread-safe since HashMap is not threadsafe and the initial get() can be called while the put() is being executed inside the synchronized block. This can break all sorts of things, so that's definitely not threadsafe.

The second solution is thread-safe, since all accesses to cache are done in guarded code. The inital get() is protected by a readlock, and the put() is done inside a writelock, guaranteeing that threads can't read the cache while it's being written to, but they're free to read it at the same time as other reading threads. No concurrency issues, no visibility issues, no chances of deadlocks. Everything's fine.

The last is of course the most elegant one. Since computeIfAbsent() is an atomic operation, it guarantees that the value is either directly returned or computed at most once, from the javadoc:

If the specified key is not already associated with a value, attempts to compute its value using the given mapping function and enters it into this map unless null. The entire method invocation is performed atomically, so the function is applied at most once per key. Some attempted update operations on this map by other threads may be blocked while computation is in progress, so the computation should be short and simple, and must not attempt to update any other mappings of this map.

The Map in question shouldn't be volatile, but it should be final. If it's not final, it could (at least in theory) be changed and it would be possible for 2 threads to work on different objects, which is not what you want.