[ACCEPTED]-Java synchronized block vs. Collections.synchronizedMap-synchronization

Collections.synchronizedMap() guarantees that each atomic operation you 4 want to run on the map will be synchronized.

Running 3 two (or more) operations on the map however, must 2 be synchronized in a block. So yes - you 1 are synchronizing correctly.

If you are using JDK 6 then you might want 2 to check out ConcurrentHashMap

Note the putIfAbsent method 1 in that class.

There is the potential for a subtle bug in your code.

[UPDATE: Since 38 he's using map.remove() this description 37 isn't totally valid. I missed that fact 36 the first time thru. :( Thanks to the question's 35 author for pointing that out. I'm leaving 34 the rest as is, but changed the lead statement 33 to say there is potentially a bug.]

In doWork() you get the 32 List value from the Map in a thread-safe 31 way. Afterward, however, you are accessing 30 that list in an unsafe matter. For instance, one 29 thread may be using the list in doWork() while another 28 thread invokes synchronizedMap.get(key).add(value) in addToMap(). Those two access are 27 not synchronized. The rule of thumb is 26 that a collection's thread-safe guarantees 25 don't extend to the keys or values they 24 store.

You could fix this by inserting a 23 synchronized list into the map like

List<String> valuesList = new ArrayList<String>();
valuesList.add(value);
synchronizedMap.put(key, Collections.synchronizedList(valuesList)); // sync'd list

Alternatively 22 you could synchronize on the map while you 21 access the list in doWork():

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

The last option will 20 limit concurrency a bit, but is somewhat 19 clearer IMO.

Also, a quick note about ConcurrentHashMap. This 18 is a really useful class, but is not always 17 an appropriate replacement for synchronized 16 HashMaps. Quoting from its Javadocs,

This 15 class is fully interoperable with Hashtable 14 in programs that rely on its thread safety 13 but not on its synchronization details.

In other words, putIfAbsent() is great 12 for atomic inserts but does not guarantee 11 other parts of the map won't change during 10 that call; it guarantees only atomicity. In 9 your sample program, you are relying on 8 the synchronization details of (a synchronized) HashMap 7 for things other than put()s.

Last thing. :) This 6 great quote from Java Concurrency in Practice always helps me in designing 5 an debugging multi-threaded programs.

For 4 each mutable state variable that may be 3 accessed by more than one thread, all accesses 2 to that variable must be performed with 1 the same lock held.

Yes, you are synchronizing correctly. I 30 will explain this in more detail. You must 29 synchronize two or more method calls on 28 the synchronizedMap object only in a case 27 you have to rely on results of previous 26 method call(s) in the subsequent method 25 call in the sequence of method calls on 24 the synchronizedMap object. Let’s take a 23 look at this code:

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);
    }
}

In this code

synchronizedMap.get(key).add(value);

and

synchronizedMap.put(key, valuesList);

method 22 calls are relied on the result of the previous

synchronizedMap.containsKey(key)

method 21 call.

If the sequence of method calls were 20 not synchronized the result might be wrong. For 19 example thread 1 is executing the method addToMap() and thread 2 is 18 executing the method doWork() The sequence of method 17 calls on the synchronizedMap object might be as follows: Thread 1 has 16 executed the method

synchronizedMap.containsKey(key)

and the result is "true". After 15 that operating system has switched execution 14 control to thread 2 and it has executed

synchronizedMap.remove(key)

After that 13 execution control has been switched back 12 to the thread 1 and it has executed for example

synchronizedMap.get(key).add(value);

believing 11 the synchronizedMap object contains the key and NullPointerException will be thrown 10 because synchronizedMap.get(key) will return null. If the sequence 9 of method calls on the synchronizedMap object is not dependent 8 on the results of each other then you don't 7 need to synchronize the sequence. For example 6 you don't need to synchronize this sequence:

synchronizedMap.put(key1, valuesList1);
synchronizedMap.put(key2, valuesList2);

Here

synchronizedMap.put(key2, valuesList2);

method 5 call does not rely on the results of the 4 previous

synchronizedMap.put(key1, valuesList1);

method call (it does not care if 3 some thread has interfered in between the 2 two method calls and for example has removed 1 the key1).

That looks correct to me. If I were to 4 change anything, I would stop using the 3 Collections.synchronizedMap() and synchronize 2 everything the same way, just to make it 1 clearer.

Also, I'd replace

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

with

List<String> valuesList = synchronziedMap.get(key);
if (valuesList == null)
{
  valuesList = new ArrayList<String>();
  synchronziedMap.put(key, valuesList);
}
valuesList.add(value);

The way you have synchronized is correct. But 20 there is a catch

1. Synchronized wrapper provided by Collection framework ensures that the method calls I.e add/get/contains will run mutually exclusive.

However in real world you 19 would generally query the map before putting 18 in the value. Hence you would need to do 17 two operations and hence a synchronized 16 block is needed. So the way you have used 15 it is correct. However.

2. You could have used a concurrent implementation of Map available in Collection framework. 'ConcurrentHashMap' benefit is

a. It has a API 'putIfAbsent' which 14 would do the same stuff but in a more efficient 13 manner.

b. Its Efficient: dThe CocurrentMap 12 just locks keys hence its not blocking the 11 whole map's world. Where as you have blocked 10 keys as well as values.

c. You could have 9 passed the reference of your map object 8 somewhere else in your codebase where you/other 7 dev in your tean may end up using it incorrectly. I.e 6 he may just all add() or get() without locking 5 on the map's object. Hence his call won't 4 run mutually exclusive to your sync block. But 3 using a concurrent implementation gives 2 you a peace of mind that it can never be 1 used/implemented incorrectly.

Check out Google Collections' Multimap, e.g. page 28 of this presentation.

If you can't 6 use that library for some reason, consider 5 using ConcurrentHashMap instead of SynchronizedHashMap; it has a nifty putIfAbsent(K,V) method 4 with which you can atomically add the element 3 list if it's not already there. Also, consider 2 using CopyOnWriteArrayList for the map values if your usage 1 patterns warrant doing so.