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 ?

full stack trace with code where you see this exception occuring? — SMA, Jul 08 '15 at 07:26
Do you synchronize the calls on the maps inside your class? Maybe these accesses may cause Problems? — red13, Jul 08 '15 at 07:34
I found the issue. One of the function (not written here) was trying to access the same object than the get instance. Those 2 functions were synchronized individually, but they could be called at the same moment To solve the issue, I changed it to an ConcurrentHashMap, and I also improved the synchronized by using Double-checked locking. Now I'll run tests the whole night and let you know if the problem is really solved — jdel, Jul 08 '15 at 14:10

score 1 · Accepted Answer · edited May 23 '17 at 12:14

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.

edited May 23 '17 at 12:14

Community

1
1

answered Jul 08 '15 at 07:28

mhlz

3,497
2
23
35

1

It seems it solved my problem, I'll do some mass test this night to be sure. Thanks a lot for all the tips – jdel Jul 08 '15 at 14:14

score 1 · Answer 2 · answered Jul 08 '15 at 07:34

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

The [Double-checked locking idiom](https://en.wikipedia.org/wiki/Double-checked_locking) alone is not a solution for this problem. The variable needs to be volatile as well. — mhlz, Jul 08 '15 at 07:42

Java Singleton Synchronization for multi-thread using HashMap

2 Answers2