title: Out of two ways to get/create items concurrently from/in ConcurrentHashMap, which one is better?
body: <p>I have found myself using two slightly different approaches for getting/creating items from/in a <code>ConcurrentHashMap</code> and I wonder which one is better.</p>
  
  <p><strong>The first way:</strong></p>
  
  <pre><code>public class Item {
    private boolean m_constructed;
    ...
    public void construct() {
      if (m_constructed) {
        return;
      }
  
      synchronized (this) {
        if (m_constructed) {
          return;
        }
  
        // Some heavy construction
  
        m_constructed = true;
      }
    }
  }
  
  ConcurrentHashMap&lt;String, Item&gt; m_items = new ConcurrentHashMap&lt;String, Item&gt;();
  
  ...
  // The following code is executed concurrently by multiple threads:
  public Item getOrCreateItem(String key) {
    Item newItem = new Item();                          // The constructor is empty
    Item item = m_items.putIfAbsent(key, newItem);
    if (item == null) {
      item = newItem;
    }
    item.construct();                                  // This is the real construction
    return item;
  }
  </code></pre>
  
  <p>Please, do not comment on using <strong>this</strong> in <code>synchronize (this)</code>. I am aware of the crappiness of using <code>this</code> as the lock object, but I am fine with it in this particular example.</p>
  
  <p><strong>The second way:</strong></p>
  
  <pre><code>public class Item {
    private boolean m_constructed;
    ...
    public void construct() {
      // Some heavy construction
  
      m_constructed = true;
    }
  
    public void waitForConstruction() throws InterruptedException {
      while (!m_constructed) {
        Thread.sleep(50);
      }
    }
  }
  
  ConcurrentHashMap&lt;String, Item&gt; m_items = new ConcurrentHashMap&lt;String, Item&gt;();
  
  ...
  // The following code is executed concurrently by multiple threads:
  public Item getOrCreateItem(String key) {
    Item newItem = new Item();                          // The constructor is empty
    Item item = m_items.putIfAbsent(key, newItem);
    if (item == null) {
      item.construct();                                 // This is the real construction
      item = newItem;
    }
    item.waitForConstruction();
    return item;
  }
  </code></pre>
  
  <p>I wonder if one way is more superior to the other. Any ideas?</p>
  
  <p><strong>EDIT</strong></p>
  
  <p>A few words on the context. The <code>Item</code> map is populated concurrently by multiple threads, all of which execute <code>getOrCreateItem</code>. No code tries to access the map in any other way. Once the population is over, the map is never modified and becomes open to read-only access. Hence no one can get a partially constructed <code>Item</code> instance outside the <code>getOrCreateItem</code> method.</p>
  
  <p><strong>EDIT2</strong></p>
  
  <p>Thanks for all the answers. I have adopted the first approach with the suggested fixes:</p>
  
  <pre><code>public class Item {
    private volatile boolean m_constructed;         // !!! using volatile
    ...
    public void construct() {
      if (m_constructed) {
        return;
      }
  
      synchronized (this) {
        if (m_constructed) {
          return;
        }
  
        // Some heavy construction
  
        m_constructed = true;
      }
    }
  }
  
  ConcurrentHashMap&lt;String, Item&gt; m_items = new ConcurrentHashMap&lt;String, Item&gt;();
  
  ...
  // The following code is executed concurrently by multiple threads:
  public Item getOrCreateItem(String key) {
    Item item = m_items.get(key);                         // !!! Checking the mainstream case first
    if (item == null) {
      Item newItem = new Item();                          // The constructor is empty
      item = m_items.putIfAbsent(key, newItem);
      if (item == null) {
        item = newItem;
      }
    }
    item.construct();                                  // This is the real construction
    return item;
  }
  </code></pre>
  
  <p>Of course, I am acting under the assumption that no code accesses the map of items using any other way but the <code>getOrCreateItem</code> method. Which is true in my code.</p>