Java Singleton Synchronization for multi-thread using HashMap

Question

I have the following class :

public class AggregationController {


    private HashMap<String, TreeMap<Integer, String>> messages; 
    private HashMap<String, Integer> counters;  
    Boolean buildAggregateReply;
    private boolean isAggregationStarted;

    private static HashMap<String, AggregationController> instances = new HashMap<String, AggregationController>();

    private AggregationController() throws MbException{
        messages = new HashMap<String, TreeMap<Integer,String>>();
        counters = new HashMap<String, Integer>();
        buildAggregateReply = true;
        isAggregationStarted = false;
    }

    public static synchronized AggregationController getInstance(String id) throws MbException{
        if(instances.get(id) == null)
            instances.put(id, new AggregationController());
        return instances.get(id);
    }   

I thought it would be enough to avoid concurrent access, but I got this error :

HashMap.java
checkConcurrentMod
java.util.HashMap$AbstractMapIterator
java.util.ConcurrentModificationException
Unhandled exception in plugin method
java.util.ConcurrentModificationException

I have 10 threads using this class, and it throws this error approximately one time every 100.000 call.

Whats is wrong with this singleton ?

Answer 1

The problem is simply that HashMaps are not thread safe as you can read in the linked docs.

You should try changing them to ConcurrentHashMaps.

Aside from that you should also change your singleton implementation to better handle multi threading. The Wikipedia page on Double-checked locking provides a lot of good examples.

p.s.: Instead of declaring your variables as HashMaps you should just declare them as Maps. That way you can change the specific implementation very easily without having to refactor anything. This is called Programming to interfaces.

Answer 2

I think the problem is with HashMap, use Concurrent HashMap

But the point I want to make is your getInstnace() function is not well written.

       public static synchronized AggregationController getInstance(String id) throws MbException{
    if(instances.get(id) == null)
        instances.put(id, new AggregationController());
    return instances.get(id);
}   

You are using synchronized on the whole method. Even thought your instance is created only one thread will be able to enter into getInstance method, Which slows down your program performance. You should do like this:

      public static AggregationController getInstance() {
           if (instance == null ) {
           synchronized (AggregationController.class) {
             if (instance == null) {
                 instance = new AggregationController();
             }
          }
      }

      return instance;
   }