Does this code solve the double checked locking issue in Java?

Question

Does this code solve the double checked locking issue in Java?

public class DBAccessService() {
    private static DBAccessService INSTANCE;  

    private DBAccessService() {}

    public static DBAccessService getInstance() {
        if (INSTANCE != null) {
            return INSTANCE;
        }
        return createInstance();
    }

    private static synchronized DBAccessService createInstance() {
        if (INSTANCE != null) {
            return INSTANCE;
        }
        DBAccessService instance = new DBAccessService();
        INSTANCE = instance;

        return INSTANCE;
    }
}

There are 2 aspects to pay attention:

1. getInstance() is not synchronized, so after INSTANCE is initialized there is no cost for synchronization
2. createInstance() is synchronized

So, the question is: does this code have any issues? Is it legal and always thread-safe?

Apple

Great question! I'd just warn you should beware of any answer that doesn't cite the JVM spec describing the Java memory model - it's too easy for people to misunderstand this – Annabel

@Ganeshkumar: erf, it's the broken DCL but at least it tries to be correct! Without any synchronization at all, you could be instantiating an arbitrary number of times a new DBAccessService! – Plumy

Answer 1

For solving this particular question Java concurrency in practice (written by the team who basically wrote the java.util.concurrent library) recommends the Lazy Initialization holder class idiom (page 348 in my copy, Listing 16.6, not 16.7)

@ThreadSafe
public class DBAccessServiceFactory {
  private static class ResourceHolder {
    public static DBAccessService INSTANCE = new DBAccessService();
  }
  public static DBAccessService getResource() {
    return ResourceHolder.INSTANCE;
  }
}

This is always legal and Thread-safe. I'm not an expert, so I can't say this is better than your code. However, given that it is the pattern recommended by Doug Lea and Joshua Bloch, I'd always use it over code you or I have invented, as it is so easy to make mistakes (as demonstrated by the number of wrong answers to this question).

Related to the volatile issue they say:

Subsequent changes in the JMM (Java 5.0 and later) have enabled DCL to work if resource is made volatile ... However the lazy initialization holder idiom offers the same benefits and is easier to understand.

Annabel

Nice answer, Nick. I've read this book, but maybe I was not prepared enough to read it. So, I was always surprised why nobody from Java gurus never recommend to use simple: 'public static final DBAccessService INSTANCE = new DBAccessService();' as was mentioned by @Kojotak too. Is it buggy? As far as I understand and research this question this is absolutely right to use, but I'm confused why it is not the recommended approach anywhere (I know that Joshua Bloch now recommends to use Enum for this purpose). – Apple

Thanks. BTW - if you like the answer it is normally polite to vote it up using the up-arrow by the question score. I don't need the reputation, so feel free not to, but it indicates to others this was a helpful answer. Similarly, if you asked the question and it was the answer you used to solve it, you can click on the check mark so "accept" it. This helps other people to know the answer was helpful – Annabel

@zshamrock: because the simple public static final ... = new ... is not lazy instantiated. The point is to be both lazy-instantiated and avoid all synchronization costs. The only clean way to do that is to use the initialize-on-demand holder class idiom (as named and explained in Effective Java). Note that using Java 1.5's volatile kinda defeats the whole point because even if you're not using the synchronize/synchronized keyword you're still adding synchronization cost (volatile is costly too). So, that's it: lazy + no-synchro: initialize-on-demand class idiom : ) – Plumy

@zshamrock - I agree exactly with SyntaxT3rr0r. If you want lazy instantiation use this pattern. If you definitely want to instantiate every time use the enum pattern (eager instantiation) – Annabel

@Plumy but isn't the above method lazy? The static field INSTANCE of ResourceHolder won't get initialized before it's actually referenced, will it? See here, under "Making it work for static Singletons Am I missing something? – Confluent

Answer 2

You need to declare INSTANCE as volatile for it to work:

private static volatile DBAccessService INSTANCE;

Note it only works with Java 5 and later. See The "Double-Checked Locking is Broken" Declaration.

Confide

Answer 3

In this article it is claimed that "double checked logging" is not an issue if you use a seperate Singleton class:

public class DBAccessHelperSingleton {
    public static DBAccessHelper instance = new DBAccessHelper(); 
} 

It has the same advantage: The field is not instantiated before being references the first time.

As stated previously, if you want to keep like you have it volatile is missing in case you are only targetting JDK >= 5.

Confluent

+1 because Bill Pugh is both a visionary and a genius ;) The dude invented the skip list. zomg :) Besides that, same note as above: using volatile defeats the whole purpose, which is both to be lazy and to avoid synchronization. – Plumy

Answer 4

It is not thread safe, please check an excelent article. There is no guarantee, that one thread will see fully initialized instance of DbAccessService. Why not using simple

public class DBAccessService() {
     public static DBAccessService INSTANCE = new DBAccessService();
}

Sportsmanship

Answer 5

Looks fine. If two threads call getInstance() and INSTANCE is not initalized, only one thread will be able to proceed with createInstance() and the second one will already see that instance is not null.

The only thing is volatile keyword for INSTANCE. Otherwise java can cache it.

Kantor

No, the code is not fine. It has exactly the same problems as the double checked locking. One way to fix it is to use the volatile keyword as explain on the second paragraph. – Medallion

ehm... do you see the fixed answer? – Kantor

@Vladimir: It's still incorrect. Volatile is not about caching, it's about memory barriers. Without volatile thread can see not-null INSTANCE pointing to a partially constructed object. – Lysin

Prooflink please. I see cache mentioning here: javamex.com/tutorials/synchronization_volatile.shtml and here: en.wikipedia.org/wiki/Volatile_variable#In_Java – Kantor

@Vladimir: See link in WhiteFang34's answer. Also see point 2 in wikipedia article - since Java 5 semantics of volatile is defined in terms of happens-before relationship, not in terms of caching. – Lysin

Ok, thanks for this. I'm not deleting the answer to save this comment thread. – Kantor