LazyReference with double-checked locking and null handling

Question

I've been using LazyReference class for a few years (not on a regular basis of course, but sometimes it is very useful). The class can be seen here. Credits go to Robbie Vanbrabant (class author) and Joshua Bloch with his famous "Effective Java 2nd edt." (original code).

The class works correctly (in Java 5+) but there is one little potential issue. If instanceProvider returns null (well it must not according to Guice Provider.get() contract, but…) then on every execution of LazyReference.get() method the LOCK will be held and instanceProvider.get will be called over and over again. It looks like a good punishment for those who break the contracts (he-he), but what if one really needs to lazily initialize a field with the possibility to set the null value?

I modified LazyReference a little bit:

public class LazyReference<T> {

  private final Object LOCK = new Object();

  private volatile T instance;

  private volatile boolean isNull;

  private final Provider<T> instanceProvider;

  private LazyReference(Provider<T> instanceProvider) {
    this.instanceProvider = instanceProvider;
  }

  public T get() {
    T result = instance;
    if (result == null && !isNull) {
      synchronized (LOCK) {
        result = instance;
        if (result == null && !isNull) {
          instance = result = instanceProvider.get();
          isNull = (result == null);
        }
      }
    }
    return result;
  }
}

IMHO it should work just fine (if you have another opinion please post your comments and criticisms). But I wonder what will happen if I remove the volatile modifier from isNull boolean (leaving it for instance of course)? Will it still work correctly?

score 3 · Answer 1 · answered Jun 22 '11 at 19:01

3

The above code has a race condition: instance may be set to the "real" null from the result of instanceProvider.get() before isNull has been set.

Are you sure it wouldn't be easier for you to just scrap this complicated nonsense and synchronise properly. I bet you will not be able to measure any difference in performance and it will be easier to verify that your code is correct.

answered Jun 22 '11 at 19:01

Neil Coffey

21,615
7
62
83

Yes you are right: `instance` may be set to "real" `null` before `isNull` is set to `true`. But in this case execution will go through synchronized block, in which `isNull` value will be already equal to 'true'. Then second conditional check will evaluate to `false` and `instanceProvider.get` won't be called more than one time. Or I'm missing something? – Volo Jun 23 '11 at 21:55
Answering your question: yes, it would be easier to use plain synchronization or use original `LazyReference` (which works flawlessly if one respects Provider contract). The purpose of question is a personal theoretical interest. **axtavt** has already posted more simple and elegant solution than mine - well, will practise more. – Volo Jun 23 '11 at 21:55

score 2 · Accepted Answer · answered Jun 22 '11 at 19:29

2

As pointed out by Neil Coffey, this code contains a race condition, but it can be easily fixed as follows (note that instance don't need to be volatile):

public class LazyReference<T> {     
  private T instance;
  private volatile boolean initialized;
  ...
  public T get() {
    if (!initialized) {
      synchronized (LOCK) {
        if (!initialized) {
          instance = instanceProvider.get();
          initialized = true;
        }
      }
    }
    return instance;
  }
}

answered Jun 22 '11 at 19:29

axtavt

239,438
41
511
482

`instance don't need to be volatile` - Good point! Since `instance` is written before volatile field `initialized` it will be visible to all other threads that read `initialized`. – Volo Jun 22 '11 at 21:42

LazyReference with double-checked locking and null handling

2 Answers2