Publishing and reading of non-volatile field

Question

public class Factory {
    private Singleton instance;
    public Singleton getInstance() {
        Singleton res = instance;
        if (res == null) {
            synchronized (this) {
                res = instance;
                if (res == null) {
                    res = new Singleton();
                    instance = res;
                }
            }
        }
        return res;
    }
}

It is almost correct implementation of thread-safe Singleton. The only problem I see is:

The thread #1 that is initializing the instance field can published before it will be initialized completely. Now, the second thread can read instance in a inconsistent state.

But, for my eye it is only problem here. Is it only problem here? (And we can make instance volatile).

Answer 1

After some time (yeah it took 2 years, I know), I think I have the proper answer. To take it literally, the answer to this:

But, for my eye it is only problem here. Is it only problem here?

Would be yes. The way you have it right now, callers of getInstance will never see a null. But if Singleton would have fields, there is no guarantee that those fields will be correctly initialized.

Let's take this slow, since the example is beautiful, IMHO. That code you showed does a single (racy) volatile read :

public class Factory {
    private Singleton instance;
    public Singleton getInstance() {
        Singleton res = instance;      // <-- volatile RACY read
        if (res == null) {
            synchronized (this) {
                res = instance;        // <-- volatile read under a lock, thus NOT racy
                if (res == null) {
                    res = new Singleton();
                    instance = res;
                }
            }
        }
        return res;
    }
}

Usually, the classical "double check locking" has two racy reads of volatile, for example:

public class SafeDCLFactory {

   private volatile Singleton instance;

   public Singleton get() {
     if (instance == null) {             // <-- RACY read 1 
       synchronized(this) {
         if (instance == null) {         // <-- non-racy read
            instance = new Singleton();
         }
       }
     }
     return instance;                    // <-- RACY read 2
   }
} 

Because those two reads are racy, without volatile, this pattern is broken. You can read how we can break here, for example.

In your case, there is an optimization, that does one less reading of a volatile field. On some platforms this matters, afaik.

The other part of the question is more interesting. What if Singleton has some fields that we need to set?

static class Singleton {
     
   //setter and getter also
   private Object obj;  
   
} 

And a factory, where Singleton is volatile:

static class Factory {

    private volatile Singleton instance;

    public Singleton get(Object obj) {
        if (instance == null) {
            synchronized (this) {
                if (instance == null) {
                    instance = new Singleton();
                    instance.setObj(obj);
                }
            }
        }
        return instance;
    }
}

We have a volatile field, we are safe, right? Wrong. The assign of obj happens after the volatile write, as such there are no guarantees about it. In plain english: this should help you a lot.

The correct way to fix this is to do the volatile write with an already build instance (fully build):

  if (instance == null) {
       Singleton local = new Singleton();
       local.setObj(obj);
       instance = local;
  }