Why are we using synchronized twice in com.iluwatar.singleton.ThreadSafeLazyLoadedIvoryTower?

[hmittal657]

`/**

• The instance doesn't get created until the method is called for the first time. */ public static synchronized ThreadSafeLazyLoadedIvoryTower getInstance() { if (instance == null) { synchronized (ThreadSafeLazyLoadedIvoryTower.class) { if (instance == null) { instance = new ThreadSafeLazyLoadedIvoryTower(); } } } return instance; }`

Shouldn't we remove synchronized from method signature? I didn't find any explaination for this. Usually for lazy initialization we have synchronized only inside the method right? What pros/cons are there for using synchronized in method signature? Code reference

[bharathkalyans]

It's called Double-Checked Locking. REFER Hope your doubt is resolved after reading the above article!

[hmittal657]

Hi @bharathkalyans thank you for reference. My doubt is actually if we already have a volatile instance, any thread will see the latest updated value of that instance. Why do we need to wrap the first null check in a synchronized block(via the method signature).

[bharathkalyans]

Please REFER to this article's second section for a better understanding of why volatile must be used!!

[hmittal657]

I understand why volatile is used here. What I didn't understand is why we need synchronized in this method signature? public static synchronized ThreadSafeLazyLoadedIvoryTower getInstance(), given that instance is already volatile, why do we need to take a lock here?

[iluwatar]

You have a good point there @hmittal657 The implementation does look a bit confusing since it mixes synchronized method with some elements from the double-checked locking. I think we should reduce it to something like this to improve clarity:

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

Note that we are trying to demonstrate different ways to implement the singleton. At the moment we have

• IvoryTower - eagerly initialized thread safe singleton
• ThreadSafeLazyLoadedIvoryTower - basic lazy loaded singleton using synchronized
• ThreadSafeDoubleCheckedLocking - lazy loaded singleton using double-checked locking
• InitializingOnDemandHolderIdiom - lazy loaded singleton using inner class
• EnumIvoryTower - lazy loaded singleton using enum

I'm also not happy with the current level of documentation we have. There is an open issue to improve it.

[bharathkalyans]

@iluwatar Do you want me to work on this?

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

[hmittal657]

I think it should be enough to remove synchronized from the method signature. We still would have double check locking and avoid expensive locking in the method calls

[iluwatar]

@hmittal657 that case is already demonstrated in ThreadSafeDoubleCheckedLocking. We don't want to duplicate it.

@bharathkalyans I'll assign you for this