Java ConcurrentHashMap actions atomicity

Question

This may be a duplicate question, but I've found this part of code in a book about concurrency. This is said to be thread-safe:

ConcurrentHashMap<String, Integer> counts = new ...;

private void countThing(String thing) {
    while (true) {
        Integer currentCount = counts.get(thing);
        if (currentCount == null) {
            if (counts.putIfAbsent(thing, 1) == null)
                break;
        } else if (counts.replace(thing, currentCount, currentCount + 1)) {
            break;
        }
    }
}

From my (concurrency beginners') point of view, thread t1 and thread t2 could both read currentCount = 1. Then both threads could change the maps' value to 2. Can someone please explain me if the code is okay or not?

Answer 1

The trick is that replace(K key, V oldValue, V newValue) provides the atomicity for you. From the docs (emphasis mine):

Replaces the entry for a key only if currently mapped to a given value. ... the action is performed atomically.

The key word is "atomically." Within replace, the "check if the old value is what we expect, and only if it is, replace it" happens as a single chunk of work, with no other threads able to interleave with it. It's up to the implementation to do whatever synchronization it needs to make sure that it provides this atomicity.

So, it can't be that both threads see currentAction == 1 from within the replace function. One of them will see it as 1, and thus its invocation to replace will return true. The other will see it as 2 (because of the first call), and thus return false — and loop back to try again, this time with the new value of currentAction == 2.

Of course, it could be that a third thread has updated currentAction to 3 in the meanwhile, in which case that second thread will just keep trying until it's lucky enough to not have anyone jump ahead of it.

Answer 2

With put you can too replace the value.

if (currentCount == null) {
        counts.put(thing, 2);
    }

Answer 3

Can someone please explain me if the code is okay or not?

In addition to yshavit's answer, you can avoid writing your own loop by using compute which was added in Java 8.

ConcurrentMap<String, Integer> counts = new ...;

private void countThing(String thing) {
    counts.compute(thing, (k, prev) -> prev == null ? 1 : 1 + prev);
}