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?

Garik

@Nathan Hughes The comments where deleted. They where mainly on my answer – Garik

What you have to watch out for is testing the return value first, outside of the synchronized block. That leads to a code path that never enters the synchronized block at all. But here you always enter the block, so there's no possible reorderings I can see that might cause problems. – Snooker

@Snooker yep, that was my initial feeling, which would mean that the first code does not need to have the return inside the synchronized. But because the devil is on the detail I am not 100% sure. – Garik

@Garik Every thread reads singleton from within a synchronized block, and the first thread also writes from the same block. At that point, singleton must be visible to any reading thread. The fact that it is also later accessed outside the block doesn't seem to matter to me, it's already guaranteed visible at that point. – Snooker

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.

Weinman

the problem I have with your reasoning is about those "barriers" that you mention. Iff that would be true, this would have not been possible. code can jump into the protected regions (and above) without problems, so the read of singleton in return instance can go above the "barrier" without any issues, as long as JMM allows such reordering. – Pah

But the JMM does not reorder operations if they cross a memory barrier. That's part of the language definition. That's one of the reasons why any memory barrier is expensive. See this link: infoq.com/articles/memory_barriers_jvm_concurrency. "A properly placed memory barrier prevents this problem by forcing the processor to serialize pending memory operations ". I don't immediately see the relevance of the page you linked @Eugene. Can you explain more? – Weinman

Also, program order ensures that the return will return the value done by the code above, again unless another write happened elsewhere. Or maybe I'm not understanding your argument. – Weinman

my point was that you mention barriers when talking about synchronized block, and if that would have been true - lock coarsening would be impossible, which it isn't. About PO I'll comment under my answer if you do not mind, not to muddy the conversion going on here – Pah

The language can coarsen the locks but memory barriers still exist -- they just get coarsened. Just because locks can be adjusted outside of loops or blocks can be combined doesn't invalidate anything that I said above. There's no way for a single thread to return null after assigning it unless another thread assigned it back to null. – Weinman

imo, it does. the fact that code can jump across barriers, implies that the read of instance in return instance can be done (speculatively) before the synchronized block is entered, let's say via a temp variable. Suppose that if (singleton == null) returns false (so that there is no write between these two reads), now you have two independent reads, that are racy. – Pah

As I mentioned @Eugene, the language must order memory events serially so that they don't cross memory barriers. The speculative read cannot happen before the read memory barrier. Regardless, program order ensures that x=1; return x; returns 1 even if you have synchronized (Foo.class) { x =1; } return x; unless x is assigned by another thread. Did you read that link I posted? – Weinman

@Pah while I prefer not to use the term “barrier”, there’s no contradiction to lock coarsening, as long as you keep in mind, which movements are possible and which barriers are responsible for the correctness. The return can be moved before the write barrier, i.e. inside the synchronized block (but we all know that placing it inside the synchronized block would be correct), but is still after the read barrier of the beginning of the synchronized block, so it still sees most recent writes of the last thread that left the synchronized block/ passed the write barrier. – Overmodest

@Pah there is a happens-before relationship between the synchronized block and any previous execution of a synchronized block using the same monitor. Then, there is a happens-before between the synchronized block and the subsequent statement within that thread due to program order. Therefore, due to transitivity, there is a happens-before between previous executions of that synchronized block and the subsequent statement. It would be racy if other executions could also write to the variable, but the logic within the synchronized block ensures that there is at most one write. – Overmodest

@Overmodest I accidentally deleted the comment, sorry about that. ' – Pah

@Overmodest thank you. I thought that if that if(singleton == null) would return false (and thus not issue singleton = new Singleton();), there would be no program order between the write in singleton and the read of it in return singleton. I was wrong. – Pah

@Pah well, there is no order between the write that did not happen and anything, but there’s still a program order between the beginning of the synchronized block, the read in the singleton == null condition and the read in the return singleton;. As long as the actual write to singleton happens-before the beginning of the synchronized block, everything works. As said, it only works because there is at most one write, definitely performed before the return. If more writes were possible, they could happen concurrently to the return statement that is outside the synchronized block. – Overmodest

@Overmodest is right. The return out of the synchronized block is correct not "since the return doesn't take any CPU or anything" (what is this all about?), but because of combination HB on object's monitor lock (synchronized) and PO between read/write in 'synchronized' and a read in 'return' (JLS 17.4.5. - "If x and y are actions of the same thread and x comes before y in program order, then hb(x, y)") with lead to transitive HB – Bulimia

The whole reason why the poster was moving it outside of the synchronized block was most likely because of performance concerns. I'll make that more clear. But why the downvotes? Is there anything in my answer that is wrong? – Weinman

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.

Pah

I would expect the following happens-before relations to exist (among others): hb(<Thread 1, write, singleton>, <Thread 1, unlock, monitor>), hb(<Thread 1, unlock, monitor>, <Thread 2, lock, monitor>), hb(<Thread 2, lock, monitor>, <Thread2, read, singleton (use by the return)>). That is, the lock of the monitor has a happens-before relation with the read for the return singleton statement (based on the program order). This would imply the return would have to see the write from the other thread, no? – Gripping

I think it is not allowed that a thread can return singleton without going through the synchronized block. Therefore it must be guaranteed that singleton is initialized. – Lobule

If you are trying to say that a single thread could set singleton and then return null, you are incorrect. There is program order that means that the return must see the assignment. You are correct if you are talking about multiple threads assigning singleton but I think you should make this much more clear. – Weinman

This analysis is incorrect. Start with one thread T; assume initially singleton is null. By the program order rule, there is a HB between the write in T and the subsequent read in T, even without the sync block. (When we add another thread into the picture, the write in T1 happens before the release of the lock, the release in T1 happens before the acquire in T2, and the acquire in T2 happens before the read in T2.) – Divisor

@BrianGoetz yeah, now I realize that. The portion that I was missing was that program order between singleton = new Singleton(); and return singleton; which brings all the needed guarantees. I somehow thought that because of that if (singleton == null) { and it was not null at the time of the check, there would be no program order. I was wrong. – Pah

@Pah Thanks for your contribution to the thread, it actually improved my understanding of things a bit. – Garik