Dagger: Double Check in Dagger

[pollux-]

  @Override
  public T get() {
    Object result = instance;
    if (result == UNINITIALIZED) {
      synchronized (this) {
        result = instance;
        if (result == UNINITIALIZED) {
          result = provider.get();
          instance = reentrantCheck(instance, result);
          /* Null out the reference to the provider. We are never going to need it again, so we
           * can make it eligible for GC. */
          provider = null;
        }
      }
    }
    return (T) result;
  }

Inside the synchronization block, why do we need to assign result = instance then check it is _UNINITIALIZED_ ?

what happens when we just ignore the assignment and check for _UNINITIALIZED_ and then assign it?

[JakeWharton]

Between Object result = instance and synchronized (this) another thread could have grabbed the lock, initialized the value, and set instance. If you don't read the field a second time when the lock is held you cannot be sure of this.

More at https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java

[bcorso]

Also, the reason why we don't ignore the assignment to result and use instance directly is just an optimization -- by assigning it to a local field first we can avoid an additional volatile read for the common path when the instance is already initialized. For example:

// This code always reads volatile field at least twice, even if the instance
// is already initialized
if (instance == UNINITIALIZED) {
  synchronized (this) {
    if (instance == UNINITIALIZED) {
      instance = reentrantCheck(instance, provider.get());
      /* Null out the reference to the provider. We are never going to need it again, so we
       * can make it eligible for GC. */
      provider = null;
    }
  }
}
return (T) instance;

[prituladima]

Does it have sense to change synchronized(this) to synchronized(private lock)?

[bcorso]

@prituladima we could but it's not free since we'd need to instantiate an object for each DoubleCheck instance. There would be a bit of extra safety by not exposing the lock, but I'm not sure it's worth the trade-off right now.

I'm going to close this since the original question was answered, but feel free to keep the conversation going or start up a new issue.