-2

I am trying to implement the following feature: each key in str array should be associated with an Integer which starts from 0 and will be stored in map. after execution the map should contains all keys in str and the count should be consistent with final value of 9. However the result varies from 8 to 12. What did I do wrong?

import java.util.HashMap;
import java.util.Map;
import java.util.Random;
import java.util.concurrent.*;
import java.util.concurrent.atomic.AtomicInteger;

public class Main {

public static final ConcurrentMap<String, Integer> map = new ConcurrentHashMap<>();
public static final AtomicInteger count = new AtomicInteger(0);
public static final String[] str = {
    "a", "b", "c", "d", "e", "f", "g", "h"
};

public static void main(String[] args) throws InterruptedException {
    for (int i = 0; i < 10; i++) {
        ExecutorService exe = Executors.newFixedThreadPool(4);
        for (int j = 0; j < 100; j++) {
            exe.execute(() -> {
                for (int k = 0; k < 1000; k++) {
                    int index = k % 8;
                    String key = str[index];
                    Integer value = map.get(key);
                    if (value == null) {
                        Integer next = count.incrementAndGet();
                        map.putIfAbsent(key, next);
                    }
                }
            });
        }
        exe.shutdown();
        exe.awaitTermination(5, TimeUnit.SECONDS);
        System.out.println("count = " + count.get());
    }


}
}
Alex Xie
  • 91
  • 1
  • 12

1 Answers1

3

You have a race condition here:

Integer value = map.get(key);   // read
if (value == null) {
    Integer next = count.incrementAndGet();
    map.putIfAbsent(key, next); // write
}

If the key is set by another thread after the read and before the write, incrementAndGet() will be executed, though it won't actually be inserted because putIfAbsent() is atomic. You can do the conditional increment atomically with respect to the map by using computeIfAbsent():

map.computeIfAbsent(key, k -> count.incrementAndGet());
shmosel
  • 49,289
  • 6
  • 73
  • 138
  • Thanks, in this case, what is usual using scenario of putIfAbsent and put? – Alex Xie Feb 02 '18 at 03:06
  • @AlexXie you can use `putIfAbsent`, if the calculation of the new value doesn’t depend on a global resource, or in general, if there is no problem if more than one thread calculate that value in parallel (if the calculation is side-effect free and cheaper than the internal synchronization of `computeIfAbsent`). Don’t worry, if you never encounter such scenarios; keep in mind, `computeIfAbsent` exists since Java 8, so from Java 5 to Java 7, `putIfAbsent` was the only way to do these updates, which required much more effort to make it correctly… – Holger Feb 13 '18 at 11:27