Singleton Pattern in Multi threaded environment

Question

During my interview, interviewer started his question with singleton pattern. I wrote below. Then, he asked Shouldn't we check for Nullity inside getInstance method?

I replied with, It is NOT necessary, since member is static type and is being initialized at the same time. But, seems like he was not satisfied with my answer.Am I correct or not ?

class Single {

        private final static Single sing = new Single();       
        private Single() {
        }        
        public static Single getInstance() {
            return sing;
        }
    }

Now, next question he ask to write singleton class for multi-threaded environment. Then, I wrote double check singleton class.

  class MultithreadedSingle {        
        private static MultithreadedSingle single;       
        private MultithreadedSingle() {
        }        
        public static MultithreadedSingle getInstance() {
            if(single==null){
                    synchronized(MultithreadedSingle.class){
                      if(single==null){
                            single= new MultithreadedSingle(); 
                              }      
                      }
                   }
             return single;
        }
    }

Then, he had an objection with using synchronized and double check and said It is useless. Why are you checking twice and why are you using synchronized ? I tried to convince him with multiple scenario. But, he didn't.

Later, at home I tried below code where I'm using simple singleton class with multiple thread.

public class Test {

    public static void main(String ar[]) {
        Test1 t = new Test1();
        Test1 t2 = new Test1();
        Test1 t3 = new Test1();
        Thread tt = new Thread(t);
        Thread tt2 = new Thread(t2);
        Thread tt3 = new Thread(t3);
        Thread tt4 = new Thread(t);
        Thread tt5 = new Thread(t);
        tt.start();
        tt2.start();
        tt3.start();
        tt4.start();
        tt5.start();

    }
}

final class Test1 implements Runnable {

    @Override
    public void run() {
        for (int i = 0; i < 5; i++) {
            System.out.println(Thread.currentThread().getName() + " : " + Single.getInstance().hashCode());
        }
    }

}
     class Single {

        private final static Single sing = new Single();       
        private Single() {
        }        
        public static Single getInstance() {
            return sing;
        }
    }

Below is the output :

Thread-0 : 1153093538
Thread-0 : 1153093538
Thread-0 : 1153093538
Thread-0 : 1153093538
Thread-0 : 1153093538
Thread-4 : 1153093538
Thread-1 : 1153093538
Thread-2 : 1153093538
Thread-3 : 1153093538
Thread-3 : 1153093538
Thread-3 : 1153093538
Thread-3 : 1153093538
Thread-3 : 1153093538
Thread-2 : 1153093538
Thread-2 : 1153093538
Thread-2 : 1153093538
Thread-2 : 1153093538
Thread-1 : 1153093538
Thread-1 : 1153093538
Thread-1 : 1153093538
Thread-1 : 1153093538
Thread-4 : 1153093538
Thread-4 : 1153093538
Thread-4 : 1153093538
Thread-4 : 1153093538

So, question is, Is it necessary to use synchronize or/and double check method in multi-threaded environment ? It seems like my first code itself (without adding any extra line of code) was the answer for both question. Any correction and knowledge share will be appreciated.

Solution 1

Your first example is absolutely correct, and is usually the preferred "idiom" for singletons. The other one is to make a single-element enum:

public enum Single {
    INSTANCE;

    ...
}

The two approaches are pretty similar unless the class is Serializable, in which case the enum approach is much easier to get right -- but if the class isn't Serializable, I actually prefer your approach the enum one, as a stylistic matter. Watch out for "accidentally" becoming Serializable due to implementing an interface or extending a class which is itself Serializable.

You are also right about the second check for nullity in the double-checked lock example. However, the sing field must be volatile for this to work in Java; otherwise, there is no formal "happens-before" edge between one thread writing to sing and another thread reading to it. This can result in that second thread seeing null even though the first thread assigned to the variable, or, if the sing instance has state, it could even result in that second thread seeing only some of that state (seeing a partially-constructed object).

Solution 2

1) Class #1 is good for multithreaded environment

2) Class #2 is a singleton with lazy initialization and double checked locking, it's a known pattern and it needs to use synchronization. But your implementation is broken, it needs volatile on the field. You can find out why in this article http://www.javaworld.com/article/2074979/java-concurrency/double-checked-locking--clever--but-broken.html

3) a Singleton with one method does not need to use lazy pattern, because its class will be loaded and initialized only at first usage.

Solution 3

Your first answer seems to be good for me, as there are no chance of a race condition whatsoever.

As for knowledge share, the best approach to implement a singleton in Java is using Enum. Create an enum with exactly one instance, and that's it. As for code sample -

public enum MyEnum {
    INSTANCE;

    // your other methods
}

From the good book Effective Java -

[....] This approach is functionally equivalent to the public field approach, except that it is much more concise, provides the serialization machinery for free, and provides an ironclad guarantee against multiple instantiation, even in the face of sophisticated serialization or reflection attacks.[...] a single-element enum type is the best way to implement a singleton.

Comments

[Ravi]

you mean to say, my first example itself was the answer of both question. Am I correct ?

[Ravi]

but for second scenario, I have used synchronized wasn't it helpful ? Assuming two thread pass first nullity, but only thread will be allowed inside synchronized and there we are again checking if nullity before initialization.Also, why should we use enum or any other method for multi-threaded environment. If we can achieve by simplest example ? :s

[yshavit]

Synchronized isn't enough, no. It's a data race (access to the same field by both writes and reads without any happens-before edge), and all sorts of crazy stuff can happen due to reorderings. Compilers and CPUs can reorder instructions to optimize performance, and when you throw a data race in, things get pretty complex and subtle. It's possible to still get it right... but the simplest thing is to just mark the field volatile.

[Kayaman]

The javaworld article is outdated and no more applies. The JMM was changed so that DCL is well defined and guaranteed to work. That said, it's better to go with the enum way since it's shorter and harder to get wrong.

[yshavit]

As for the enum approach, it's mostly stylistic. The serialization issue comes across if someone serializes your singleton and then deserializes it -- now there are two instances, and it's not a singleton! Again, there are ways to guard against that, but even there it can get a bit tricky; the easiest thing is to mark it an enum and let the JVM handle serialization instance-resolving for you.

[harmonica141]

Why does that solve the problem. Please explain and help the op learn.