Is the lockless lazy loading pattern used in Guava really thread-safe?

Question

Some Guava internal types, like AbstractMultiset, have a pattern like this:

private transient Set<E> elementSet;

@Override
public Set<E> elementSet() {
  Set<E> result = elementSet;
  if (result == null) {
    elementSet = result = createElementSet();
  }
  return result;
}

Set<E> createElementSet() {
  return new ElementSet();
}

The idea is to delay creating the collection views (elementSet(), entrySet()) until they're actually needed. There's no locking around the process because if two threads call elementSet() at the same time, it's okay to return two different values. There will be a race to write the elementSet field, but since writes to reference fields are always atomic in Java, it doesn't matter who wins the race.

However, I worry about what the Java memory model says about inlining here. If createElementSet() and ElementSet's constructor both get inlined, it seems like we could get something like this:

@Override
public Set<E> elementSet() {
  Set<E> result = elementSet;
  if (result == null) {
    elementSet = result = (allocate an ElementSet);
    (run ElementSet's constructor);
  }
  return result;
}

This would allow another thread to observe a non-null, but incompletely initialized value for elementSet. Is there a reason that can't happen? From my reading of JLS 17.5, it seems like other threads are only guaranteed to see correct values for final fields in elementSet, but since ElementSet ultimately derives from AbstractSet, I don't think there's a guarantee that all its fields are final.

Answer 1

I'm not 100% clear on this (I'm sure someone else on our team could answer this better). That said, a couple thoughts:

1. I don't think we claim anywhere that this is (guaranteed to be) thread-safe. Non-thread-safe collections such as HashMultiset extend AbstractMultiset. That said, ConcurrentHashMultiset also extends AbstractMultiset and uses its implementation of elementSet(), so presumably it must in fact be possible for it to be thread-safe.
2. I believe the thread safety of this method is dependent on the implementation of createElementSet(). From what I can tell, if the Set created by createElementSet() is immutable (in that the fields that are assigned when it is constructed are final), it should be thread-safe. This appears to be true in the case of ConcurrentHashMultiset at least.

Edit: I asked Jeremy Manson about this, and he said: "Your take on it seems fine to me. It isn't thread safe. If the object being constructed has all of the final fields in the right places, you should be fine, but I wouldn't rely on that by accident (note that many implementations are effectively immutable instead genuinely immutable)."

Note: For thread-safe collections like ConcurrentHashMultiset which use this pattern, objects created are intentionally and genuinely immutable (though if AbstractSet were to change, that could change, as Chris noted in the comments).