double check locking on HashMap - thread safe?

Question

Is the following code thread safe? The class SlidingTimeWindowCountGauge is already thread safe. The question is around the double check locking code + the regular non-concurrent hashmap.

I would like to lazily create entries in the hashmap. Entries should only be created once and reused thereafter. I would like to avoid locking if possible.

public class InstrumentedCaller {
    private final Map<String, SlidingTimeWindowCountGauge> nameToCountGauge = new HashMap<>();

    private SlidingTimeWindowCountGauge getGaugeLazy(final String name) {
        SlidingTimeWindowCountGauge gauge = nameToCountGauge.get(name);
        if (gauge != null) {
            return gauge;
        }
        synchronized (this) { 
            if (nameToCountGauge.containsKey(name)) { 
                return nameToCountGauge.get(name);
            }
            final SlidingTimeWindowCountGauge newGauge = new SlidingTimeWindowCountGauge(1, TimeUnit.MINUTES);
            this.nameToCountGauge.put(name, newGauge);
            return newGauge;
        }
    }

    private void markCall(final String callName) {
        SlidingTimeWindowCountGauge gauge = getGaugeLazy(callName);
        gauge.mark();
    }

    public void doCall1() {
       markCall("call1");
    }

    public void doCall2() {
       markCall("call2");
    }
}

No, it is not thread-safe, since you access the map without synchronizing. There is no *happens-before* relation between the `put` and the `get`, so although the `get` may retrieve a non-null reference from the `Map`, the referenced object may not have transferred between threads. — Andreas, Oct 16 '18 at 21:55
Suppose a put() triggered a rehash. In the middle of the rehash the get() is called. That probably would cause a concurrency issue. But what if the rehash() was implemented by doing an atomic variable swap at the end causing the get() to get stale data from the old hashtable. For this code a stale null would still work fine since it would hit the synchronized block. Could you elaborate more on the specific error this code suffers from? — bradforj287, Oct 16 '18 at 22:01
Is there a reason you wouldn't just use ConcurrentHashMap here? Seems unlikely you'll improve on it. — Nathan Hughes, Oct 16 '18 at 22:03
You're right, the re-hash is another way it is not thread-safe, but I was referring to the *referenced* `SlidingTimeWindowCountGauge` object not being guaranteed to be available to the other thread, even if the *reference* to it is available in the map. Since the map and the object resides in difference memory areas, they may flush to memory independently of each other, so thread 2 may see the reference *before* it sees the object itself. — Andreas, Oct 16 '18 at 22:04
I am using the ConcurrentHashMap ;) This came up in a code review. The only change was to switch HashMap -> ConcurrentHashMap. The thought was that change made it thread safe. I am posting this as an educational question to examine the precise low level reason why the code is not thread safe. — bradforj287, Oct 16 '18 at 22:06
See: [How to solve the “Double-Checked Locking is Broken” Declaration in Java?](https://stackoverflow.com/q/3578604/5221149) — Andreas, Oct 16 '18 at 22:13
See: [Why is double-checked locking broken in Java?](https://stackoverflow.com/q/4926681/5221149) — Andreas, Oct 16 '18 at 22:15
Or just do a web search for [`java double check locking broken`](https://www.google.com/search?q=java+double+check+locking+broken) — Andreas, Oct 16 '18 at 22:16
My understanding is that for the Gauge not being available to the other thread problem, the ConcurrentHashmap solves that by using a volatile variable in the internal node class for the backing table http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/util/concurrent/ConcurrentHashMap.java — bradforj287, Oct 16 '18 at 22:19

score 3 · Accepted Answer · answered Oct 16 '18 at 22:08

Correct thread-safe implementation of getGaugeLazy using ConcurrentHashMap and computeIfAbsent:

private final Map<String, SlidingTimeWindowCountGauge> nameToCountGauge = new ConcurrentHashMap<>();

private SlidingTimeWindowCountGauge getGaugeLazy(final String name) {
    return nameToCountGauge.computeIfAbsent(name, k -> new SlidingTimeWindowCountGauge(1, TimeUnit.MINUTES));
}

Notice how it is also much simpler to write.

double check locking on HashMap - thread safe?

1 Answers1