Caching wrapper for digest computation

Question

I need to review this code:

import java.util.HashMap;
import java.util.Map;

// REVIEW
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);
}

1. Should I mark cache as volatile ?
   I have doubts because we use cashe.get in method body instead of direct cashe

2. Would it make sense to replace the map with ConcurrentHashMap?

3. Is there way to fix code without replacing map implementation?

Answer 1

Volatile won't do anything here, as this basically only affects the reference to the map, not the contents of the map.

As for ConcurrentHashMap (as opposed to explicit synchronizing I suppose): this depends. If you want to make absolutely sure that no digest is ever computed twice, stick to your approach. If you just want general concurrency, but can accept that eventually in race-conditions, digest is called twice for the same input (sidenote: I suppose digest is deterministic, so that two calls with the same input yield the same result), you may simply repace the map with a ConcurrentHashMap and the whole method with:

return cache.computeIfAbsent(input, this::doDigest);