Singleton pattern

Question

The following snippet is straight forward,

public MyClass getInstance() {
    if(uniqueInstance == null) {
        uniqueInstance = new MyClass();
    }
    return uniqueInstance;
}

What does the following one do?

public MyClass getInstance() {
    if(uniqueInstance == null) {
        synchronized(MyClass.class) {
            uniqueInstance = new MyClass();
        }
    }
    return uniqueInstance;
}

score 5 · Accepted Answer · answered Dec 05 '10 at 05:39

5

It's a poor attempt to make it threadsafe to prevent a race condition caused by at least two threads which have simultaneously entered the if block.

A bit more safe approach is adding an extra nullcheck, also known as double-checked locking.

public MyClass getInstance() {
    if (uniqueInstance == null) {
        synchronized(MyClass.class) {
            if (uniqueInstance == null) {
                uniqueInstance = new MyClass();
            }
        }
    }
    return uniqueInstance;
}

However, I usually prefer Just Create One pattern over Singleton.

answered Dec 05 '10 at 05:39

BalusC

1,082,665
372
3,610
3,555

"A bit more safe"... I wouldn't be too sure of that: http://www.google.ca/search?q=double+checked+locking+is+broken – Karl Knechtel Dec 05 '10 at 06:05
1

@Karl: hence the *"a bit"*. It's also explained in the linked Wikipedia article. – BalusC Dec 05 '10 at 06:06

score 3 · Answer 2 · answered Dec 05 '10 at 05:38

This is a bit of insurance for thread safety.

From the javaworld article here:

Synchronizing the method guarantees that a call to the method cannot be interrupted.

The main idea is that if you don't have the synchronized block it is possible for 2 threads to call the getInstance and reinitializing the object, thereby potentially losing any state data (if you should even have state data in a singleton)

score 3 · Answer 3 · answered Dec 05 '10 at 11:12

IMHO You should start with the simplest options first. The simplest singleton is an enum with one entry. Given that classes are load lazily, this will still give you lazy loading, unless to refer to the class directly and this doesn't happen by accident easily.

enum Singleton {
   INSTANCE;
}

To avoid accident loading you can use an inner class.

class Singleton {
    static class SingeltonHolder {
        static final Singleton INSTANCE = new Singleton();
    }
    public static Singleton getInstance() {
        return SingletonHolder.INSTANCE;
    }
}

Note; neither solution needs synchronization because they use the fact that class loading is thread safe.

In short; There are situations where locking is required, but don't make it more complicated than it needs to be.

score 1 · Answer 4 · answered Dec 05 '10 at 05:40

The second one does the same thing, except: if you're using the first one, what happens if creating a new MyClass takes some amount of time, and during that time, someone else also calls MyClass.getInstance()? You could potentially end up with two instances. The second version locks the instantiation line, so that if another class tries to call at the same time, it will wait until the first is done.

Singleton pattern

4 Answers4

Linked