Is using a synchronised block of concurrent hash map correct?

Question

As per my understanding ConcurrentHashMap will allow multiple threads to read and write (add/remove) on the same hash map with out getting concurrent hash map exception.

I have 4 threads and each thread can update the hashmap. I do not want other threads to write/update on hashmap while the current thread is updating it.

ConcurrentHashMap<String, Integer> playerLoginCounterHashMap = new ConcurrentHashMap<>();

    ExecutorService executorService = Executors.newFixedThreadPool(4);

    for (int i = 0; i < 4; i++) {

        executorService.submit(new Runnable() {
            @Override
            public void run() {
                synchronized (playerLoginCounterHashMap) {
                    if (playerLoginCounterHashMap.get("testPlayer") == null) {
                        playerLoginCounterHashMap.put("testPlayer", 1);
                    } else {
                        playerLoginCounterHashMap.put("testPlayer", playerLoginCounterHashMap.get("testPlayer").intValue() + 1);
                    }
                }
            }
        });
    }

Is this the correct way of doing it? Without synchronised block I get the values which are not correct.

Answer 1

Yes, it's correct (assuming this is the only place where the map is updated), but it's inefficient, since it synchronizes instead of relying on the inherent non-blocking concurrency of the map.

You should use compute() instead:

playerLoginCounterHashMap.compute(
    "testPlayer",
    (key, value) -> value == null ? 1 : value + 1);

Or merge():

playerLoginCounterHashMap.merge(
    "testPlayer",
    1,
    Integer::sum);