Hashmap updates not reflected between threads

Question

I have a singleton class that contains a hahsmap, the hashmap is initialised as a class variable. This map is updated correctly because when i add and print the size it changed, but, when i call it from a different thread the map is always empty. Is there a particular reason why this might happen?

I am using a ConccurentHashMap if this makes any difference.

Thanks

Singleton decleration:

    public class ClientRegistryDetailsSingleton {

        private static ClientRegistryDetailsSingleton instance = null;
        private ConcurrentHashMap<String, Integer> tickerToNumberRegistered = new ConcurrentHashMap<String,Integer>();


        protected ClientRegistryDetailsSingleton() {
             // Exists only to defeat instantiation.
        }

        public static ClientRegistryDetailsSingleton getInstance() {
            if(instance == null) {
               instance = new ClientRegistryDetailsSingleton();
            }
            return instance;
        }

        public void setTickerToNumberRegistered(ConcurrentHashMap<String, Integer> tickerToNumberRegistered) {
            this.tickerToNumberRegistered = tickerToNumberRegistered;
        }

        public ConcurrentHashMap<String, Integer> getTickerToNumberRegistered() {
            return tickerToNumberRegistered;
        }


    public void addToClienets(String ticker){}

    public void removeFromClients(String ticker){}
}

Calling it from another thread:

String[] splitForTicker = message.split(",");

        ConcurrentHashMap<String, Integer> map = ClientRegistryDetailsSingleton.getInstance().getTickerToNumberRegistered();
        System.out.println("The number of items in the map from senders persepctive" + map.size());

Output:

The number of items in the map from senders persepctive 0
2012-11-12 14:29:12,495 [Process messages received] INFO  com.feed.feedReceive.ProcessFeedStreamLine - Successfully received a message from the feed
The number of items in the map from senders persepctive 0
1 :the size of the map now someone has added
2012-11-12 14:29:15,495 [Process messages received] INFO  com.feed.feedReceive.ProcessFeedStreamLine - Successfully received a

message from the feed The number of items in the map from senders persepctive 0

New code for Singleton

public class ClientRegistryDetailsSingleton {

    private static ClientRegistryDetailsSingleton instance = new ClientRegistryDetailsSingleton();
    private volatile ConcurrentHashMap<String, Integer> tickerToNumberRegistered = new ConcurrentHashMap<String,Integer>();

    protected ClientRegistryDetailsSingleton() {
         // Exists only to defeat instantiation.
    }

    public static synchronized ClientRegistryDetailsSingleton getInstance() {
        return instance;
    }

    public synchronized ConcurrentHashMap<String, Integer> getTickerToNumberRegistered() {
        return tickerToNumberRegistered;
    }

    public void addToClienets(String ticker){}

    public void removeFromClients(String ticker){}
}

Answer 1

There is a race condition in the posted code that can result in more that one instance of the singleton being constructed if two threads call getInstance() and the singleton has not yet been constructed:

public static ClientRegistryDetailsSingleton getInstance() {
    if(instance == null) {                                   // Line 1
        instance = new ClientRegistryDetailsSingleton();     // Line 2
    }
}

A possible execution of two threads, T1 and T2:

• T1 peforms check at line 1 and enters if branch.
• T1 is suspended, with instance still being null.
• T2 peforms check at line 1 and enters if branch.
• T2 constructs class and assigns to instance.
• T2 returns instance to caller.
• T2 is suspended.
• T1 is started again and constructs another instance and assigns to instance.

The construction of the single instance must be threadsafe. Possible solutions would be:

• Make the getInstance() method synchronized.

• Don't use lazy initialization (if possible):

  private static final ClientRegistryDetailsSingleton instance =
      new ClientRegistryDetailsSingleton();
  

Answer 2

Define your hash map as

private volatile ConcurrentHashMap<String, Integer>

The volatile keyword warns the JVM that the state of the variable may be changed by another thread at any time, so it must not be cached locally.

Defining the instance also as volatile might also be necessary.

Answer 3

Implementation of DCL antipattern: http://en.wikipedia.org/wiki/Double_checked_locking

public class ClientRegistryDetailsSingleton {

    private static volatile ClientRegistryDetailsSingleton instance = null;

    private final ConcurrentHashMap<String, Integer> tickerToNumberRegistered = new ConcurrentHashMap<String,Integer>();


    private ClientRegistryDetailsSingleton() {
         // Exists only to defeat instantiation. 
         // please not that constructor should be private
    }

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

    //You should not break encapsulation and allow a link to HashMap escape
    private void setTickerToNumberRegistered(ConcurrentHashMap<String, Integer> tickerToNumberRegistered) {
        this.tickerToNumberRegistered = tickerToNumberRegistered;
    }

    //You should not break encapsulation and allow a link to HashMap escape
    private ConcurrentHashMap<String, Integer> getTickerToNumberRegistered() {
    }

     //I omitted the access to the hash map it's likely that some additional params required
     public void addToClienets(String ticker){}

     //I omitted the access to the hash map it's likely some additional params are required
     public void removeFromClients(String ticker){}
}

I've shown this only as a synchronization example, in real life you should likely implement your singletons as Enums : What is the best approach for using an Enum as a singleton in Java?