1

After reading dozens of articles about DCL. I feel that I should not use this concept without volatile.If I will not lead this technique my code will not thread save and very very bad according one hundreed different reasons.
Recently I reread basics Happens Before and I have a bit another view
Lets research singleton code listing:

public class Singleton{
    private static Something instance = null;
    public static Singleton getInstance() {
        if (instance == null) { // point 1
            synchronized (Singleton.class) {
                if (instance == null)
                    instance = new Something();
            }
        }
        return instance;  //point 2
    }

}

we change instance only inside synchronized (Something.class) {

thus we will see actual value inside synchronized section which uses same monitor. It is our case.

Thus now I have suspicion that this is not effective but it thread safe.

Am I right?

Only one concern:

Can if (instance == null) { to see non null value before actual assignment instance = new Something();

But I still absolutely sure that this code doesn't allow to create 2 singleton instances

P.S.

I read a bit more and looks like if in point 1 we read non-null value, return instance at point 2 could return null;

gstackoverflow
  • 36,709
  • 117
  • 359
  • 710
  • It's equivalent to `if(instance!=null) return instance;`. Because there's no synchronization action at all here, no `happens-before` relationship is established on this thread (regardless of the `synchronized` action in another thread), therefore all bets are off. – ZhongYu Jan 31 '17 at 16:01
  • @Zhong Yu As I understand it is posiible that first read can return non-null value then second read can return null value according JMM. I don't know about implementation but looks like specification allow this. – gstackoverflow Jan 31 '17 at 17:00
  • Yes. That can be fixed by reading `instance` once into a local variable. – ZhongYu Jan 31 '17 at 17:03
  • @Zhong Yu local variables fixed this because this doesn't share between threads? – gstackoverflow Jan 31 '17 at 17:05
  • JMM is only concerned with shared variables. Local variables all obey inner thread sequential consistency, their values will not change mysteriously (or we'll all lose our jobs). – ZhongYu Jan 31 '17 at 17:14
  • Note that if `instance` is an immutable object, it doesn't have to be `volatile`. Nevertheless, the problem of reading non-null then null is still there, so we must read it to a local variable first. – ZhongYu Jan 31 '17 at 17:16

1 Answers1

2

The problem in your example is not in possible creating of two instances. That's true that only one instance will be created. Real problem is that on multiple thread access to this method, other thread can start using partially constructed version of instance (1)(2).

So, instance variable should be definitely defined as volatile (which is missed in your code block), otherwise you should concern about "freshness" of this variable's value.

Because there is no locking if the field is already initialized, it is critical that the field be declared volatile (Item 66) [J. Bloch, "Effective Java", Item 71]

So:

private static volatile Something instance;

(BTW, explicitly assigning null value is redundant).

Why it doesn't work without "volatile" is good explained here:

The most obvious reason it doesn't work it that the writes that initialize the Helper object and the write to the helper field can be done or perceived out of order

Community
  • 1
  • 1
Andremoniy
  • 34,031
  • 20
  • 135
  • 241