1

Suppose we are instantiating a singleton using double-checked locking:

public static Instance getInstance() {
    if (this.instance == null) {
        synchronized(Instance.class) {
            if (this.instance == null) {
                this.instance = new Instance();
            }
        }
    }
    return this.instance;
}

The question is in the semantics of the program if instance variable would be volatile and double-checked locking would be removed.

private volatile static Instance instance;

public static Instance getInstance() {
    if (this.instance == null) {
        this.instance = new Instance();
    }
    return this.instance;
}

Will the class get instantiated only once? Or, put another way, can volatile reads clash in such a way that two threads will see null value of the reference and double instantiation will be performed?

I am aware of the happens-before relationship between volatile write and volatile read, and that volatile forbids caching (so all reads and writes will be executed in main memory, not in processor's cache), but it isn't clear in the case of concurrent volatile reads.

P.S.: the question is not in applying of the Singleton pattern (it was just an example where the problem is clear), it is just about if double-checked locking can be replaced with volatile read - volatile write without program semantics change, nothing more than that.

nyarian
  • 2,915
  • 9
  • 36
  • Possible duplicate of [How to solve the "Double-Checked Locking is Broken" Declaration in Java?](https://stackoverflow.com/questions/3578604/how-to-solve-the-double-checked-locking-is-broken-declaration-in-java) – Shloim Oct 22 '18 at 13:14
  • Volatile means once the Instance class is intantiated and assigned to the instance variable the null checks will return false. But it doesn't stop you from getting two null checks at once and as such you might get a double instantiation. You should add synchronized to the getInstance method, this way you have a cleaner code and the assurances you need. – Tomaz Fernandes Oct 22 '18 at 13:15

4 Answers4

2

Without synchronization, your code is definitely broken, because 2 threads may see that the value of instance is null, and both will perform an initialization (consider a context switch at every line, and see what happens.

Beside that, even with synchronization double checked locking (DCL) was considered broken in Java in the past because when running unsynchronized, a second thread may experience the initialization operations in a different order. You may fix your code by adding a local variable and load the volatile into it whenever you want to read it:

public static Instance getInstance() {
    Instance tmp = instance;
    if (tmp == null) {
        synchronized(Instance.class) {
            Instance tmp = instance;
            if (tmp == null) {
                instance = new Instance();
            }
        }
    }
    return instance;
}

but a safer solution would be using the ClassLoader as your synchronization mechanism, and will also allow you to stop using the slow volatile access every time you access the singleton:

public class Instance {

    private static class Lazy {
        private static Instance INSTANCE = new Instance();    
    }

    public static Instance getInstance() {
        return Lazy.INSTANCE;
    }
}

INSTANCE will be initialized only when the first thread enters getInstance()

Shloim
  • 4,781
  • 16
  • 32
  • Why would it be lazy instantiated? Being a static member wouldn’t it be instantiated at application start? – Tomaz Fernandes Oct 22 '18 at 13:40
  • yes, I've read something about initialization via ClassLoader in Concurrency In Practice if I remember it right... And yes, I've just missed atomicity between checking and initialization (shame...) Thanks anyway, the answer was very helpful! – nyarian Oct 22 '18 at 13:44
  • 1
    static members are initialized when the class is accessed for the first time, not when the application starts. The Lazy class is there in order to delay the initialization to the first time we access `getInstance()` and not the first time we access `class Instance`. – Shloim Oct 22 '18 at 13:53
  • yes, I know about dynamic loading, but thanks anyway!:) – nyarian Oct 23 '18 at 08:28
  • sorry, it was an answer to @TomazFernandes – Shloim Oct 23 '18 at 08:29
  • oops, sorry, missed that – nyarian Oct 23 '18 at 08:31
2

Yes, indeed: volatile reads can clash in such way that two threads will see null value of the reference and double instantiation will be performed.

You need double brace initialization and volatile as well. That's because when instance becomes non null, you don't synchronize other threads on anything before reading it - first if just makes them go further to return the unsynchronized value (even if the initializing thread doesn't escape the synchronized block yet), this could result in a subsequent thread reading not initialized variable because of the lack of synchroniaztion. Synchronization to work properly needs to be performed by every thread accessing the data governed by it, the DCL omits the synchronization after initialization which is wrong. That's why you need additionaly volatile for the DCL to work, then the volatile will ensure that you read initialized value.

There is no such thing as processor cache separation, reads and writes are visible right away, but there are instruction rearrangements, so in favor for optimization processor can invoke some instructions later in time if it doesn't need their results right away. The whole point of synchronization and volatile is to not rearrange the order of instructions for threads accesing them. That way if something is synchronized and is stated as done in code, it is really done and other threads can access it safely. That's the whole point of happens before guarantee.

Summing this together: without proper synchronization processor can initialize the reference to the instance to be non null, but instance could be not fully initialized internally, so subsequent thread reading it could read uninitialized object and behave wrongly because of that.

Krzysztof Cichocki
  • 5,658
  • 1
  • 13
  • 31
  • Before your explanation of instructions rearrangement process I didn't really understand it and thought of it as a completely magical thing. Moreover, happens-before rules were something I just remember as a list of rules where visibility is guaranteed. Thank you, now I do understand what is going on under the hood! But anyway here is a question, because what I know about processors caching is that all values are being cached in the processor for any thread, whereas entering the synchronized block synchronizes all values with main memory, and exiting the block flushes cache into main memory – nyarian Oct 23 '18 at 08:10
  • and I've actually wrote a little program where first thread sets a flag in an object to true, second thread starts and executes empty cycle while the flag is set to true. But when first thread sets the flag to false and uses `if` operator with this value (here we are actually confident that even reordering was performed, the assignment was actually invoked on demand before `if` clause), second thread continues to execute empty cycle and the program doesn't stop. But when I inserted System.out.println() invocation inside the cycle (which uses synchronized block) - second threads exits the cycle – nyarian Oct 23 '18 at 08:22
  • here's the code snippet: `final Holder flag = new Holder<>(true); new Thread(() -> { while (flag.value) {} System.out.println("ENDED"); }).start(); Thread.sleep(100L); flag.value = false; Thread.sleep(100L); if (flag.value) { Thread.sleep(500L); } else { Thread.sleep(100L); }` – nyarian Oct 23 '18 at 08:32
  • That's because if you don't do proper synchronization the instruction could be not executed at all because of the rearrangements and compiler is also free to do optimizations. Making proper synchronization is a key for your program to work correctly. Otherwise program could behave strangly, you have proved it. It is hard to tell what was the exact mechanism of incorrect behaviour because it is very complex, but this should't bother you, read a book "Java Concurrency in Practice" to learn how to write working concurrent code - it is simpler. – Krzysztof Cichocki Oct 24 '18 at 13:52
  • If you think that my answer helped you to understand the subject better then consider marking it as the correct one. – Krzysztof Cichocki Oct 24 '18 at 13:53
  • that's answer about reordering, isn't it? – nyarian Oct 24 '18 at 14:02
  • Yes and no :) the whole thing about concurrency is exactly about instruction reordering + some other optimization cases, but mainly because the reordering. – Krzysztof Cichocki Oct 25 '18 at 06:56
1

Considering this code.

private volatile static Instance instance;

public static Instance getInstance() {
    if (this.instance == null) {
        this.instance = new Instance();
    }
    return this.instance;
}

From your question:

Will the class get instantiated only once? Can volatile reads clash in such way that two threads will see null value of the reference and double instantiation will be performed?

Volatile reads can't clash in such a way out of JMM guarantees. You can still end up with two instances though, if multiple threads swap after the if but before beginning to instantiate the volatile variable. 

if (this.instance == null) {
    // if threads swap out here you get multiple instances
    this.instance = new Instance();
}

In order to ensure the above scenario does not happen, you have to use double checked locking 

if (this.instance == null) {
    // threads can swap out here (after first if but before synchronized)
    synchronized(Instance.class) {
        if (this.instance == null) {
            // but only one thread will get here
            this.instance = new Instance();
        }
    }
}

Note that there are two aspects that have to be considered here.

  • Atomicity: we need to make sure that the second if and the instantiation happen in an atomic manner (this is why we need the synchronized block).
  • Visibility: we need to make sure that the reference to the instance variable does not escape in an inconsistent state (this is why we need the volatile declaration for the instance variable in order to leverage the JMM happens before guarantee).
  • god, missed atomicity. shame on me... but what if I would replace volatile instance with AtomicReference? Can I just remove double-checked locking? – nyarian Oct 22 '18 at 13:34
  • @AndreyIlyunin No, doesn't look like it https://stackoverflow.com/questions/5938163/singleton-using-atomicreference –  Oct 22 '18 at 13:37
  • Why pay the extra cost of volatile or atomic when you can do it without any of them? – Shloim Oct 22 '18 at 13:54
  • @Shloim from OPs question `P.S.: the question is not in applying of the Singleton pattern (it was just an example where the problem is clear), it is just about if double-checked locking can be replaced with volatile read - volatile write without program semantics change, nothing more than that.` –  Oct 22 '18 at 13:57
0

Use an enum if you really need a singleton in Java. It fixes these things for you:

enum MySingleton {
    INSTANCE;

    public static MySingleton getInstance() {
        return INSTANCE;
    }
}

The JVM will initialize the instance in a thread safe manner on the first access to MySingleton. The only way to get more than one instance of it, is if you have multiple classloaders.

marstran
  • 19,484
  • 2
  • 45
  • 58
  • Correction: it will initialize the instance on the first mention of `MySingleton`. If you want it to initialize it only on the first access to `getInstance()`, you'll need an inner `static enum` inside `class MySingleton` – Shloim Oct 22 '18 at 13:26
  • @Shloim True, fixed. – marstran Oct 22 '18 at 13:28