Repository: iluwatar/java-design-patterns
Issue: Why are we using synchronized twice in com.iluwatar.singleton.ThreadSafeLazyLoadedIvoryTower?
Labels: type: bug
type: support
epic: pattern

`/**
   * 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](https://github.com/iluwatar/java-design-patterns/blob/master/singleton/src/main/java/com/iluwatar/singleton/ThreadSafeLazyLoadedIvoryTower.java#L46)

Issue comments:
It's called **Double-Checked Locking**. [REFER](https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java) 
Hope your doubt is resolved after reading the above article!

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).
Please [REFER](http://jeremymanson.blogspot.com/2008/11/what-volatile-means-in-java.html) to this article's second section for a better understanding of why volatile must be used!!
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?
You have a good point there @hmittal657 The [implementation](https://github.com/iluwatar/java-design-patterns/blob/master/singleton/src/main/java/com/iluwatar/singleton/ThreadSafeLazyLoadedIvoryTower.java) 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](https://github.com/iluwatar/java-design-patterns/issues/187) to improve it.

@iluwatar Do you want me to work on this?
```
public static synchronized ThreadSafeLazyLoadedIvoryTower getInstance() {
    if (instance == null) {
        instance = new ThreadSafeLazyLoadedIvoryTower();
    }
    return instance;
}
```
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
@hmittal657 that case is already demonstrated in ThreadSafeDoubleCheckedLocking. We don't want to duplicate it.

@bharathkalyans I'll assign you for this

Pull request review comments:

Patch that resolved the issue:
diff --git a/singleton/src/main/java/com/iluwatar/singleton/ThreadSafeLazyLoadedIvoryTower.java b/singleton/src/main/java/com/iluwatar/singleton/ThreadSafeLazyLoadedIvoryTower.java
index 58d6e53b9729..fcafba3611c5 100644
--- a/singleton/src/main/java/com/iluwatar/singleton/ThreadSafeLazyLoadedIvoryTower.java
+++ b/singleton/src/main/java/com/iluwatar/singleton/ThreadSafeLazyLoadedIvoryTower.java
@@ -44,13 +44,9 @@ private ThreadSafeLazyLoadedIvoryTower() {
    * 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) {
+      if (instance == null) {
           instance = new ThreadSafeLazyLoadedIvoryTower();
-        }
       }
-    }
     return instance;
   }
 }


Test patch: