In a Thread Safe Singleton does the return have to be inside the synchronized block

Question

Consider the following code:

private static Singleton singleton;

public static Singleton get(){
    synchronized (Singleton.class) {  
        if (singleton == null) {  
            singleton = new Singleton();  
        }  
    } 
    return singleton; // <-- this part is important
}

This comes as a follow-up discussion from this question. Initially, I thought that it was thread-safe. However, some respectable users argue that is not thread-safe because of the return singleton outside the synchronized block. Some other also (respectable) users, however, argued otherwise.

After I have read do we need volatile when implementing singleton using double-check locking, I changed my mind. (The code from that question):

    private static Singleton instance;

    private static Object lock = new Object();

    public static Singleton getInstance() {
        if(instance == null) {
            synchronized (lock) {
                if(instance == null) {
                    instance = new Singleton();
                }
            }
        }
        return instance;
    }

(It is well-known why the volatile is needed on the second code.)

However, after looking again at both examples, I have noticed that there is a big difference between the first and the second code snippets. On the former the outermost if is inside the synchronized clause therefore all the threads running within the synchronized block will force a happen-before relation (i.e., there is no way threads will return null if the instance was properly set) Or am I wrong? I would expect the following order of actions:

lock monitor
...
unlock monitor
...
read singleton

I have noticed that all the examples online that are similar to the first code snippet have the return inside the synchronized block; However, that can be simply because performance-wise it is the same since threads have to synchronized away, so why not be on the safe side and put the return inside?!.

Question:

Does the return really need to be inside the synchronized block? Can the read of the singleton value for the return statement see a value of the singleton before the synchronized block start?

Answer 1

Does the return really needs to be inside the synchronized block?

No the return does not need to be in the synchronized block unless the singleton field can be assigned elsewhere. However, there is no good reason why the return shouldn't be inside of the synchronized block. If the entire method is wrapped in a synchronized then you can just mark the method as synchronized if we are in the Singleton class here. This would be cleaner and better in case singleton gets modified elsewhere.

In terms of why it doesn't need to be inside, since you are using a synchronized block, there is a read-barrier crossed at the start of the block and a write-barrier at the end, meaning that the threads will get the most up-to-date value of singleton and it will only be assigned once.

The read memory barrier ensures that the threads will see an updated singleton which will either be null or a fully published object. The write memory barrier ensures that any updates to singleton will be written to main memory which includes the full construction of Singleton and the publishing of it to the singleton field. Program order guarantees that the singleton assigned within the synchronized block will be returned as the same value unless there is another assignment in another thread to singleton then it will be undefined.

Program order would be more in force if you did something like the following. I tend to do this when singleton is volatile (with appropriate double-check locking code).

synchronized (Singleton.class) {
    Singleton value = singleton;
    if (singleton == null) {
       value = new Singleton();
       singleton = value;
    }
    return value;
}

not thread-safe because of the return singleton outside the synchronized block

Since you are using a synchronized block, this isn't an issue. The double check locking is all about trying to avoid the synchronized block being hit on every operation as you point out.

all the threads running within the synchronized block will force a happen-before relation (i.e., there is no way threads will return null if the instance was properly set) Or am I wrong?

That's correct. You aren't wrong.

However, that can be simply because performance-wise it is the same since threads have to synchronized away, so why not be on the safe side and put the return inside?!.

No reason not to although I would argue that the "safe side" is more about causing consternation when others review this code and are worrying about it in the future, as opposed to being "safer" from the standpoint of the language definition. Again, if there are other places where singleton is assigned then the return should be inside of the synchronized block.

Answer 2

EDIT : I was wrong in the initial answer, but I will keep it to show where my mistake was.

there is a program order between the write in the singleton = new Singleton(); and the read in return singleton, which establishes the needed guarantees; i.e.: this is safe.

---

between this: if (singleton == null) and this singleton = new Singleton() there is program order relationship, which according to the JLS, brings also a happens-before order.

But this write: singleton = new Singleton(); has no relationship at all with this read : return singleton;, which is a racy read. JLS says that in case of such racy reads, nothing is guaranteed. So even if you wrote new Singleton() to singleton, there is no guarantee that return singleton will read that written value; it can still read null. The guarantee is only there when reading happens under the same lock.

Making singleton volatile fixes that problem, because you now create a synchronizes-with order against : singleton = new Singleton() and return singleton, which implicitly creates a happens-before now.

This is how I see it.