java - Out of two ways to get/create items concurrently from/in ConcurrentHashMap, which one is better? -

i have found myself using 2 different approaches getting/creating items from/in concurrenthashmap , wonder 1 better.

the first way:

public class item {   private boolean m_constructed;   ...   public void construct() {     if (m_constructed) {       return;     }      synchronized (this) {       if (m_constructed) {         return;       }        // heavy construction        m_constructed = true;     }   } }  concurrenthashmap<string, item> m_items = new concurrenthashmap<string, item>();  ... // following code executed concurrently multiple threads: public item getorcreateitem(string key) {   item newitem = new item();                          // constructor empty   item item = m_items.putifabsent(key, newitem);   if (item == null) {     item = newitem;   }   item.construct();                                  // real construction   return item; } 

please, not comment on using this in synchronize (this). aware of crappiness of using this lock object, fine in particular example.

the second way:

public class item {   private boolean m_constructed;   ...   public void construct() {     // heavy construction      m_constructed = true;   }    public void waitforconstruction() throws interruptedexception {     while (!m_constructed) {       thread.sleep(50);     }   } }  concurrenthashmap<string, item> m_items = new concurrenthashmap<string, item>();  ... // following code executed concurrently multiple threads: public item getorcreateitem(string key) {   item newitem = new item();                          // constructor empty   item item = m_items.putifabsent(key, newitem);   if (item == null) {     item.construct();                                 // real construction     item = newitem;   }   item.waitforconstruction();   return item; } 

i wonder if 1 way more superior other. ideas?

edit

a few words on context. item map populated concurrently multiple threads, of execute getorcreateitem. no code tries access map in other way. once population over, map never modified , becomes open read-only access. hence no 1 can partially constructed item instance outside getorcreateitem method.

edit2

thanks answers. have adopted first approach suggested fixes:

public class item {   private volatile boolean m_constructed;         // !!! using volatile   ...   public void construct() {     if (m_constructed) {       return;     }      synchronized (this) {       if (m_constructed) {         return;       }        // heavy construction        m_constructed = true;     }   } }  concurrenthashmap<string, item> m_items = new concurrenthashmap<string, item>();  ... // following code executed concurrently multiple threads: public item getorcreateitem(string key) {   item item = m_items.get(key);                         // !!! checking mainstream case first   if (item == null) {     item newitem = new item();                          // constructor empty     item = m_items.putifabsent(key, newitem);     if (item == null) {       item = newitem;     }   }   item.construct();                                  // real construction   return item; } 

of course, acting under assumption no code accesses map of items using other way getorcreateitem method. true in code.

i think first solution not bad. m_constructed variable must volatile work correctly, , john vint suggested, calling concurrentmap.get() before doing putifabsent() recommended.

i suspect important call path steady state access (threads accessing item added map , constructed). in case, first solution, concurrenthashmap.get() call , volatile read (on m_constructed), not bad.

the second solution poor 1 involves unnecessary busy spin loop. when converted using countdownlatch per john vint's suggestion, steady state performance similar above: concurrenthashmap.get() call , countdownlatch.await() should similar uncontended volatile read. however, downside adds more memory item.