Why to synchronize on SynchronizedMap or SynchronizedCollections?

Question

I am referring to question asked here and using authors code example, now my question is

1. Why does author uses synchronized(synchronizedMap), is it really necessary because synchronizedMap will always make sure that there are no two threads trying to do read/put operation on Map so why do we need to synchronize on that map itself?

Would really appreciate an explanation.

---

  public class MyClass {
  private static Map<String, List<String>> synchronizedMap =
      Collections.synchronizedMap(new HashMap<String, List<String>>());

  public void doWork(String key) {
    List<String> values = null;
    while ((values = synchronizedMap.remove(key)) != null) {
      //do something with values
    }
  }

  public static void addToMap(String key, String value) {
    synchronized (synchronizedMap) {
      if (synchronizedMap.containsKey(key)) {
        synchronizedMap.get(key).add(value);
      }
      else {
        List<String> valuesList = new ArrayList<String>();
        valuesList.add(value);
        synchronizedMap.put(key, valuesList);
      }
    }
  }
}

Answer 1

why do we need to synchronize on that synchronizemap itself?

You may need to synchronize on an already synchronized collection because you are performing two operations on the collection -- in your example, a containsKey() and then a put(). You are trying to protect against race conditions in the code that is calling the collection. In addition, in this case, the synchronized block also protects the ArrayList values so that multiple threads can add their values to these unsynchronized collections.

If you look at the code you linked to, they first check for the existence of the key and then put a value into the map if the key did not exist. You need to protect against 2 threads checking for a key's existence and then both of them putting into the map. The race is which one will put first and which one will overwrite the previous put.

The synchronized collection protects itself from multiple threads corrupting the map itself. It does not protect against logic race conditions around multiple calls to the map.

synchronized (synchronizedMap) {
    // test for a key in the map
    if (synchronizedMap.containsKey(key)) {
      synchronizedMap.get(key).add(value);
    } else {
      List<String> valuesList = new ArrayList<String>();
      valuesList.add(value);
      // store a value into the map
      synchronizedMap.put(key, valuesList);
   }
}

This is one of the reasons why the ConcurrentMap interface has the putIfAbsent(K key, V value);. That does not require two operations so you may not need to synchronize around it.

Btw, I would rewrite the above code to be:

synchronized (synchronizedMap) {
    // test for a key in the map
    List<String> valuesList = synchronizedMap.get(key);
    if (valueList == null) {
      valuesList = new ArrayList<String>();
      // store a value into the map
      synchronizedMap.put(key, valuesList);
    }
    valuesList.add(value);
}

Lastly, if most of the operations on the map need to be in a synchronized block anyway, you might as well not pay for the synchronizedMap and just use a HashMap always inside of synchronized blocks.

Answer 2

It is not just about updating synchronizedMap values, it is about sequence of operations effecting the map. There are two operations happening on the map inside same method.

If you don't synchronize block/method, assume there may be case like Thread1 executing first part and thread2 executing second part, your business operation may results in weird results (even though updates to map are synchronized)