Java synchronized block vs. Collections.synchronizedMap

user_75 0
Viewed 111281

Is the following code set up to correctly synchronize the calls on synchronizedMap?

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

From my understanding, I need the synchronized block in addToMap() to prevent another thread from calling remove() or containsKey() before I get through the call to put() but I do not need a synchronized block in doWork() because another thread cannot enter the synchronized block in addToMap() before remove() returns because I created the Map originally with Collections.synchronizedMap(). Is that correct? Is there a better way to do this?

synchronization java
7 Answers

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

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

user_245450

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

Note the putIfAbsent method in that class.

TofuBeer_658680

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

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

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

You could fix this by inserting a 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 you could synchronize on the map while you 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 limit concurrency a bit, but is somewhat clearer IMO.

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

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

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

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

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

JLR_448300

That looks correct to me. If I were to change anything, I would stop using the Collections.synchronizedMap() and synchronize everything the same way, just to make it 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);
Paul_Tomblin_33330

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

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

Barend_494890
Related