Which one is a better singleton and why?

Question

I was asked in an interview to check which of the following 2 ways of declaring singleton class is better with proper reasons.Can anybody please share some ideas

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

OR

public static synchronized Singleton getSingleInstance() {
    if (singleInstance == null) {
        singleInstance = new Singleton();
    }
    return singleInstance;
}

http://en.wikipedia.org/wiki/Singleton_pattern Wikipedia explains much better than anywhere else — spiderman, Feb 20 '14 at 04:18
My vote for #2 - cause the `null` check is done within the context of the synchronised block...A better solution would have being to use a `enum`... — MadProgrammer, Feb 20 '14 at 04:19
The first one would work better. http://en.wikipedia.org/wiki/Double-checked_locking — anonymous, Feb 20 '14 at 04:19
@rocking But there's nothing stopping two threads from evaluating the outer check at the same time, it introduces more code and possible misunderstandings. A better solution would be to simply remove the outer `null` check altogether - This is just my opinion — MadProgrammer, Feb 20 '14 at 04:24
@rocking It also depends on what it is you are trying to achieve. Is it being done for efficiency in a high threaded, demanding environment, where the reference to to the instance is short lived by the caller for example, then the first one would be more "efficient", but from a readability POV, the second is better, `enum` is the final work to both — MadProgrammer, Feb 20 '14 at 04:27
Double checked locking in Java is broken. Have a look at http://www.javaworld.com/article/2074979/java-concurrency/double-checked-locking--clever--but-broken.html — Karthik Kalyanasundaram, Feb 20 '14 at 04:28
@KarthikKalyanasundaram, it's not broken if the singleton variable is defined as `volatile`. — SimonC, Feb 20 '14 at 04:39

score 0 · Answer 1 · answered Feb 20 '14 at 04:29

0

As this is an interview question I think their intention goes to #1, as it avoids synchronization for most of the time.

answered Feb 20 '14 at 04:29

Markus Malkusch

7,738
2
38
67

score 0 · Answer 2 · answered Feb 20 '14 at 04:48

The answer depends on what is more important, performance or code legibility. For performance, the first is better [2], but for legibility, the second is better as the fewer lines of code are easier to understand and prove are free of bugs.

[2] Performance will depend on the speed of volatile reads compared to acquiring a monitor on the class defining the singleton method, assuming the method is called more than once. I don't have any quantitative proof, but I would assume the former is more better.

For what it's worth, the Apache commons LazyInitializer uses the first (http://commons.apache.org/proper/commons-lang/apidocs/src-html/org/apache/commons/lang3/concurrent/LazyInitializer.html):

@Override
public T get() throws ConcurrentException {
    // use a temporary variable to reduce the number of reads of the
    // volatile field
    T result = object;

    if (result == null) {
        synchronized (this) {
            result = object;
            if (result == null) {
                object = result = initialize();
            }
        }
    }

    return result;
}

score 0 · Answer 3 · answered Feb 20 '14 at 06:21

Depends entirely on your needs.

In this case -- outside of the problem the first has of not properly protecting the test (your if should be inside the synchronization, or you risk race conditions where someone else changes it after you've tested it but before you've finished acting on the result of the test), so you could wind up with two instances -- the two are probably equivalent after the JIT is done with them.

In general, however, the problem with synchronized methods tends to be that they're oversynchronized. Many of them hold onto their lock longer than absolutely necessary, and may cost you performance by forcing other threads to wait longer. Synchronization should be done for the minimum time necessary to make the operation atomic, and often that can be limited to a few key lines rather than the whole method.

On The Other Hand... sometimes it's best not to make the class threadsafe at all. Hashtables were threadsafe; HashMaps (the newer alternative) aren't. That's because getting the lock is a relatively expensive process, especially in multiprocessor environments, and in real-world code if threadsafety is needed it generally wants to cover more than the single hashmap access so locking at this level is more often wasteful than helpful.

Which one is a better singleton and why?

3 Answers3