I wrote a below Singleton class. I am not sure whether this is thread safe singleton class or not?
public class CassandraAstyanaxConnection { private static CassandraAstyanaxConnection _instance; private AstyanaxContext<Keyspace> context; private Keyspace keyspace; private ColumnFamily<String, String> emp_cf; public static synchronized CassandraAstyanaxConnection getInstance() { 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; } }
Can anyone help me with this? Any thoughts on my above Singleton class will be of great help.
Updated Code:-
I am trying to incorporate Bohemian suggestion in my code. Here is the updated code, I got-
public class CassandraAstyanaxConnection { private static class ConnectionHolder { static final 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; } }
Can anyone take a look and let me know if this time I got it right or not?
Thanks for the help.
Advertisement
Answer
You are implementing the lazy initialization pattern – where the instance is created when first used.
But there is a simple trick that allows you to code a threadsafe implementation that doesn’t require synchronization! It is known as the Initialization-on-demand holder idiom, and it looks like this:
public class CassandraAstyanaxConnection { private CassandraAstyanaxConnection(){ } private static class Holder { private static final CassandraAstyanaxConnection INSTANCE = new CassandraAstyanaxConnection(); } public static CassandraAstyanaxConnection getInstance() { return Holder.INSTANCE; } // rest of class omitted }
This code initializes the instance on the first calling of getInstance()
, and importantly doesn’t need synchronization because of the contract of the class loader:
- the class loader loads classes when they are first accessed (in this case
Holder
‘s only access is within thegetInstance()
method) - when a class is loaded, and before anyone can use it, all static initializers are guaranteed to be executed (that’s when
Holder
‘s static block fires) - the class loader has its own synchronization built right in that make the above two points guaranteed to be threadsafe
It’s a neat little trick that I use whenever I need lazy initialization. You also get the bonus of a final
instance, even though it’s created lazily. Also note how clean and simple the code is.
Edit: You should set all constructors as private or protected. Setting and empty private constructor will do the work