1

I've been studying for a job interview and I've found this question:

What's wrong with the following Singleton factory method getInstance()?

public class Singleton {

    private static Singleton mySingleton;

    protected Singleton() {}

    public static Singleton getInstance() {
        if (mySingleton == null) {
           synchronized(Singleton.class) {
             if(mySingleton == null) {
                mySingleton = new Singleton();
             }
           }
        }
        return mySingleton
    }   
}

I know the constructor is wrong and should be private. However, they're asking about what's wrong with the getInstance() method, everything looks fine to me, I've seen a lot of examples like this one (in a multithreading environement) what am I missing?

Pshemo
  • 122,468
  • 25
  • 185
  • 269
SaintLike
  • 9,119
  • 11
  • 39
  • 69

4 Answers4

4

The largest problem with the original code is that it is a classic example of the Double Check anti-pattern. It looks great but is just broken. More on that here: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

Note that as of java 1.5 there is a way to finesse it, the synchronized pattern or better static initialization is really the way to go.

-edit (I just noticed this is studying for a job interview. Pulling out "double check" if they show this example would be good. If asked for the details, I would go with complex java memory issues you did not fully understand but accept the fact to avoid double check. Good luck)

BrianT.
  • 394
  • 1
  • 6
1

This does appear to be thread-safe, however the code is more complex than it needs to be. Plus the use of a lock, comes with a performance cost. It takes time to acquire the lock, and other threads attempting to execute that code are blocked.

These 2 concerns could be addressed by using a static initializer and removing synchronization from the getInstance method entirely. The JVM will ensure that this statement is executed exactly once. The result is significantly simpler code, and slightly more-performant code.

public class Singleton {

    private static Singleton mySingleton = new Singleton();

    private Singleton() {}

    public static Singleton getInstance() {
        return mySingleton;
    }   
}
EJK
  • 12,332
  • 3
  • 38
  • 55
  • how come? if one thread enters the synchronized block shouldn't it be locked? – SaintLike Nov 10 '14 at 02:54
  • 2
    The are two `if(mySingleton == null)` statements to get through, one inside the lock. – President James K. Polk Nov 10 '14 at 02:55
  • You are correct. I did not see the 2nd conditional. It is thread-safe. It is just more complex than it needs to be. I will correct my answer. – EJK Nov 10 '14 at 03:02
  • Furthermore... its just bad Java grammar. – shinjw Nov 10 '14 at 03:02
  • Hmm ok, it could be simpler but there's nothing wrong with the method, right? – SaintLike Nov 10 '14 at 03:05
  • 1
    Depends on what is meant by "wrong". The code will work, it will not be the most performant. – EJK Nov 10 '14 at 03:06
  • Okay, okay. No need to shout. – EJK Nov 10 '14 at 04:57
  • @MyNameIsRui This answer isn't the correct answer. That is, it won't answer your question correctly. BrianT's answer is more correct. The interview question is asking 'What happens when multiple threads invoke `getInstance` assuming we are lazy-loading?' Look up lazy-initialization anti-pattern (DCL). – John Vint Nov 11 '14 at 22:12
0

what am I missing?

Possible memory effects described in JLS 17, so there is no happens-before relationship between write and read to mySingleton. It is counterintuitive, but if you checked that mySingleton is not null, you can see wrong object. I.e. mySingleton in this example can be written and read at the same time, it is called data-race. To fix this program you can just add volatile. According specifications it creates happens-before relationship between write and read.

Denis Borovikov
  • 737
  • 5
  • 9
-1

Isn't what you have a bit too redundant? How about this instead?

public static synchronized Singleton getInstance()
    {
        if (mySingleton == null)
            mySingleton = new Singleton();

        return mySingleton;
    }

or...

public static synchronized Singleton getInstance()
    {
        return mySingleton==null?mySingleton=new Singleton():mySingleton;
    }

compliments to Mr. Polywhirl

shinjw
  • 3,329
  • 3
  • 21
  • 42