Potential problems in Double Checked locking pattern in Singleton class

Question

I believe the below Singleton class that I have wrote is Thread Safe.

The double-checked locking pattern might run into problems in some circumstances apparently (I've seen people warn against it, though it was a while ago so I'd just be Googling for an answer now)

I am not sure right now, whether there will be any problem with the Double Checked locking pattern in my below Singleton class. I added the double checked locking patter to make the program run faster slightly.

    public class CassandraAstyanaxConnection {

        private static CassandraAstyanaxConnection _instance;
        private static final Object syncObject = new Object();
        private AstyanaxContext<Keyspace> context;
        private Keyspace keyspace;
        private ColumnFamily<String, String> emp_cf;


       public static CassandraAstyanaxConnection getInstance() {
            if (_instance == null) {
                     synchronized(syncObject) {
                        if (_instance == null) {
                            _instance = new CassandraAstyanaxConnection();
                        }
                   }
            }
              return _instance;
       }

        /**
         * Creating Cassandra connection using Astyanax client
         *
         */
        private CassandraAstyanaxConnection() {

            context = new AstyanaxContext.Builder()
            .forCluster(ModelConstants.CLUSTER)
            .forKeyspace(ModelConstants.KEYSPACE)
            .withAstyanaxConfiguration(new AstyanaxConfigurationImpl()      
                .setDiscoveryType(NodeDiscoveryType.RING_DESCRIBE)
            )
            .withConnectionPoolConfiguration(new ConnectionPoolConfigurationImpl("MyConnectionPool")
                .setPort(9160)
                .setMaxConnsPerHost(1)
                .setSeeds("127.0.0.1:9160")
            )
            .withAstyanaxConfiguration(new AstyanaxConfigurationImpl()      
                .setCqlVersion("3.0.0")
                .setTargetCassandraVersion("1.2"))
            .withConnectionPoolMonitor(new CountingConnectionPoolMonitor())
            .buildKeyspace(ThriftFamilyFactory.getInstance());

            context.start();
            keyspace = context.getEntity();

            emp_cf = ColumnFamily.newColumnFamily(
                ModelConstants.COLUMN_FAMILY, 
                StringSerializer.get(), 
                StringSerializer.get());
        }

        /**
         * returns the keyspace
         * 
         * @return
         */
        public Keyspace getKeyspace() {
            return keyspace;
        }

        public ColumnFamily<String, String> getEmp_cf() {
            return emp_cf;
        }
    }

Does my code have any problem with Double checked locking pattern in my above Singleton class?

And what's the best way to create a Singleton class thread safe. And what about the Holder Class Idiom? Can I use that here in my Singleton class?

Any example basis on my above Singleton class will help me to understand better how to write better thread safe singleton class.

Thanks for the help.

Updated Code:-

After some suggestion, I have made changes in the code-

public class CassandraAstyanaxConnection {

   private static class ConnectionHolder {
       public static CassandraAstyanaxConnection connection = new CassandraAstyanaxConnection()
   }

   public static CassandraAstyanaxConnection getInstance() {
        return ConnectionHolder.connection;
   }

   /**
    * Creating Cassandra connection using Astyanax client
    *
    */
    private CassandraAstyanaxConnection() {

        context = new AstyanaxContext.Builder()
        .forCluster(ModelConstants.CLUSTER)
            .forKeyspace(ModelConstants.KEYSPACE)
            .withAstyanaxConfiguration(new AstyanaxConfigurationImpl()      
                .setDiscoveryType(NodeDiscoveryType.RING_DESCRIBE)
            )
            .withConnectionPoolConfiguration(new ConnectionPoolConfigurationImpl("MyConnectionPool")
                .setPort(9160)
                .setMaxConnsPerHost(1)
                .setSeeds("127.0.0.1:9160")
            )
            .withAstyanaxConfiguration(new AstyanaxConfigurationImpl()      
                .setCqlVersion("3.0.0")
                .setTargetCassandraVersion("1.2"))
            .withConnectionPoolMonitor(new CountingConnectionPoolMonitor())
            .buildKeyspace(ThriftFamilyFactory.getInstance());

            context.start();
            keyspace = context.getEntity();

            emp_cf = ColumnFamily.newColumnFamily(
                ModelConstants.COLUMN_FAMILY, 
                StringSerializer.get(), 
                StringSerializer.get());
        }

        /**
         * returns the keyspace
         * 
         * @return
         */
        public Keyspace getKeyspace() {
            return keyspace;
        }

        public ColumnFamily<String, String> getEmp_cf() {
            return emp_cf;
        }
}

Let me know if this looks right.

Answer 1

I am a big fan of the following pattern for lazy-initialized singletons:

public final class CassandraAstyanaxConnection {

   ...

   private static class ConnectionHolder {
       public static CassandraAstyanaxConnection connection = new CassandraAstyanaxConnection()
   }

   public static CassandraAstyanaxConnection getInstance() {
        return ConnectionHolder.connection;
   }

   ...
}

Answer 2

Your DCL is broken, _instance must be volatile to work correctly

private static volatile CassandraAstyanaxConnection _instance;

In your case the best solution is

private static CassandraAstyanaxConnection _instance = new CassandraAstyanaxConnection();
...
public static CassandraAstyanaxConnection getInstance() {
    return _instance;
}

since getInstance() is the only public method it is lazy. Only when you call getInstance() class is loaded and instance is created. And this is what Holder idiom is about. In your case you dont need it. The whole class works as that idiom

Answer 3

The best way to create a thread safe singleton class is by using an enum

public enum CassandraAstyanaxConnection {

       INSTANCE;
       //fields

        public void initializeConnection() {
            //Move work from private constructor here.
        }


}

Something like this should work I suppose.

Answer 4

If you want to do the lazy initialization on the static field, the best pattern is "lazy initialization with a holder class":

public class CassandraAstyanaxConnection {
    private static class ConnectionHolder {
       static final CassandraAstyanaxConnection field = 
              new CassandraAstyanaxConnection();
    }

    public static CassandraAstyanaxConnection getInstance() { 
         return ConnectionHolder.field; 
    }
...
}

In this case the lazy initialization is guaranteed by the JVM, the field is not initialized until the ConnectionHolder class is class loaded.

Answer 5

A good singleton:

1- Make the class final.

2- Make the static instance final.

3- Override the readResolve() to return the instance.

4- For perfection , throw exception from clone().

5- synchronize the getInstance().

6- Make the default constructor private.

Follow this to see if the double check locking idiom actually works or not .

The best way is using an enum. Something of this sort:

public enum DBConn{
 CON_INSTANCE;
 private CassandraAstyanaxConnection connection;
 public synchronized CassandraAstyanaxConnection getConnection(){
   if(connection == null){
      // instantiate a connection
   }
   return connection;
 }
}

Access it by DBConn.CON_INSTANCE.getConnection();