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?

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.

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.

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.

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();
}

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.