Search code examples
javamultithreadingvolatileconcurrenthashmaphappens-before

With double-checked locking, does a put to a volatile ConcurrentHashMap have happens-before guarantee?


So far, I have used double-checked locking as follows:

class Example {
  static Object o;
  volatile static boolean setupDone;

  private Example() { /* private constructor */ }

  getInstance() {
    if(!setupDone) {
      synchronized(Example.class) {
        if(/*still*/ !setupDone) {
          o = new String("typically a more complicated operation"); 
          setupDone = true;
        }        
      }
    }
    return o;
  }
}// end of class

Now, because we have groups of threads that all share this class, we changed the boolean to a ConcurrentHashMap as follows:

class Example {
  static ConcurrentHashMap<String, Object> o = new ConcurrentHashMap<String, Object>();
  static volatile ConcurrentHashMap<String, Boolean> setupsDone = new ConcurrentHashMap<String, Boolean>();

  private Example() { /* private constructor */ }

  getInstance(String groupId) {
    if (!setupsDone.containsKey(groupId)) {
      setupsDone.put(groupId, false);
    }
    if(!setupsDone.get(groupId)) {
      synchronized(Example.class) {
        if(/*still*/ !setupsDone.get(groupId)) {
          o.put(groupId, new String("typically a more complicated operation")); 
          setupsDone.put(groupId, true); // will this still maintain happens-before?
        }        
      }
    }
    return o.get(groupId);
  }
}// end of class

My question now is: If I declare a standard Object as volatile, I will only get a happens-before relationship established when I read or write its reference. Therefore writing an element within that Object (if it is e.g. a standard HashMap, performing a put() operation on it) will not establish such a relationship. Is that correct? (What about reading an element; wouldn't that require to read the reference as well and thus establish the relationship?)

Now, with using a volatile ConcurrentHashMap, will writing an element to it establish the happens-before relationship, i.e. will the above still work?

Update: The reason for this question and why double-checked locking is important: What we actually set up (instead of an Object) is a MultiThreadedHttpConnectionManager, to which we pass some settings, and which we then pass into an HttpClient, that we set up, too, and that we return. We have up to 10 groups of up to 100 threads each, and we use double-checked locking as we don't want to block each of them whenever they need to acquire their group's HttpClient, as the whole setup will be used to help with performance testing. Because of an awkward design and an odd platform we run this on we cannot just pass objects in from outside, so we hope to somehow make this setup work. (I realise the reason for the question is a bit specific, but I hope the question itself is interesting enough: Is there a way to get that ConcurrentHashMap to use "volatile behaviour", i.e. establish a happens-before relationship, as the volatile boolean did, when performing a put() on the ConcurrentHashMap? ;)


Solution

  • Yes, it is correct. volatile protects only that object reference, but nothing else.

    No, putting an element to a volatile HashMap will not create a happens-before relationship, not even with a ConcurrentHashMap.

    Actually ConcurrentHashMap does not hold lock for read operations (e.g. containsKey()). See ConcurrentHashMap Javadoc.

    Update:

    Reflecting your updated question: you have to synchronize on the object you put into the CHM. I recommend to use a container object instead of directly storing the Object in the map:

    public class ObjectContainer {
        volatile boolean isSetupDone = false;
        Object o;
    }
    
    static ConcurrentHashMap<String, ObjectContainer> containers = 
        new ConcurrentHashMap<String, ObjectContainer>();
    
    public Object getInstance(String groupId) {
      ObjectContainer oc = containers.get(groupId);
      if (oc == null) {
        // it's enough to sync on the map, don't need the whole class
        synchronized(containers) {
          // double-check not to overwrite the created object
          if (!containers.containsKey(groupId))
            oc = new ObjectContainer();
            containers.put(groupId, oc);
          } else {
            // if another thread already created, then use that
            oc = containers.get(groupId);
          }
        } // leave the class-level sync block
      }
    
      // here we have a valid ObjectContainer, but may not have been initialized
    
      // same doublechecking for object initialization
      if(!oc.isSetupDone) {
        // now syncing on the ObjectContainer only
        synchronized(oc) {
          if(!oc.isSetupDone) {
            oc.o = new String("typically a more complicated operation"));
            oc.isSetupDone = true;
          }        
        }
      }
      return oc.o;
    }
    

    Note, that at creation, at most one thread may create ObjectContainer. But at initialization each groups may be initialized in parallel (but at most 1 thread per group).

    It may also happen that Thread T1 will create the ObjectContainer, but Thread T2 will initialize it.

    Yes, it is worth to keep the ConcurrentHashMap, because the map reads and writes will happen at the same time. But volatile is not required, since the map object itself will not change.

    The sad thing is that the double-check does not always work, since the compiler may create a bytecode where it is reusing the result of containers.get(groupId) (that's not the case with the volatile isSetupDone). That's why I had to use containsKey for the double-checking.