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;
}
