Thread Safe singleton class

Question

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.

Dump

Yes it it thread-safe, as long as the Keyspace and ColumnFamily class are too. It's not optimal however, you're going to get a lot of contention on the class synchronization in getInstance(). You should look up the "enum singleton pattern". – Shoring

see javarevisited.blogspot.in/2012/07/… and #428402 and maybe answer your own question with new enum version of your class ty @eurobur for pointing this out – Sinful

#71189 – Threaten

I was searching if it's ok to call a fetch logic to populate the private members of the singleton class. I found this example which is clearly invoking additional APIs to fill the private members. I always thought that you should do that outside the singleton class. Please provide your feedback. – Balder

Answer 1

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 the getInstance() 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

Jointworm

why would your class be accessed before getInstance is called? have other static methods? – Sinful

what i mean is if you have a singleton why would you want to expose other static methods? and if not then why would the class be initialized except when someone called getInstnace? – Sinful

@Sinful I didn't spell it out (but I have edited to do so). The class whose initialization I'm talking about is the static inner class Holder, not the outer class CassandraAstyanaxConnection. This implementation doesn't change the API, except to remove the synchronized keyword from getInstance(), thus improving performance of access to the instance. – Jointworm

"private static inner class static field" -- there is an official name for that, Initialization-on-demand holder / IODH – Assentor

this is the best impl over all. okay another class in the runtime but dont know how much more mem that will take then a check and sync every time you call getInstance – Sinful

How much memory can it take? It has just one static field and nothing else. A few bytes at most. – Jointworm

@Bohemian, Thanks for the suggestion. I am assuming this is the right way to go for the singleton thread safe class and also after reading various article on the internet also, I think this is correct way. So I have updated my question with the latest code. Can you please take a look and let me know if that is correct or not? Thanks for the help. – Dump

Your code is perfect :) And yes, I believe this pattern to be the cleanest, fastest singleton pattern that us threadsafe. You're welcome for the help. Good luck with your IT journey. – Jointworm

Brian Goetz et al., Java Concurrency in Practice (2006), 348. – Rora

What if you need to pass a parameter to getInstance()? – Tani

@bstar passing a parameter implies that different instances will be returned depending on the parameter, which is the factory method pattern, not the singleton pattern. – Jointworm

@Jointworm not necessarily, what if the thing I'm passing in is the context of my application (Android) – Tani

@Tani What if another call is made with a different parameter? If only one parameter is intended to be passed in, then create another method like static synchronized initialize(MyParam x) that may only be called once, and have getInstance() return the instance created in initialize() – Jointworm

It is causing the following violation: Accessor Class Generation - Avoid instantiation through private constructors from outside of the constructor's class. – Upmost

@Upmost that "violation" can only be from some style checking tool (eg checkstyle). There is nothing wrong with the code; it compiles and executes OK. – Jointworm

Yes, that's right. I just mentioned it in case someone have the same issue. I still am trying to figure out how to solve that violation. – Upmost

@Upmost IMHO it's a false positive and the tool should be configured, or the line marked, to ignore this instance of the rule. The class invoking the constructor is a private inner class, which is part of the class itself. You could do a work around using a factory method: Within Holder do INSTANCE = MyClass.create(); and add private static MyClass create() {return new MyClass();}, but that would be perfunctory; done to sidestep an arbitrary style rule but adding no real value. Better to ignore it. – Jointworm

What if another instance is created through Class cls = Class.forName("com.test.Test"); Object object = cls.newInstance(); – Oliver

@Oliver Any protective code can be circumvented using reflection. Using reflection, you can instantiate a Void and mutate a String. That doesn't mean "the code is broken". – Jointworm

Question is about thread safety of the singleton if I'm correct. And the answer is about how to create a Singleton thread safely at the creation time. It does not guarantee the use of that singleton is about to be thread safe. Actually OP's singleton is also a valid implementation sometime ago. But the safely instantiated singleton will be a thread safe singleton only if both keyspace and emp_cf are thread safe (ex. immutable). – Scriven

Here I need to add one point, it's still possible to create another instance using clone method. So we can improve this good implementation. – Sadye

I was searching if it's ok to call a fetch logic to populate the private members of the singleton class. I found this example which is clearly invoking additional APIs to fill the private members. I always thought that you should do that outside the singleton class. Please provide your feedback. – Balder

@Balder you can have as much code as needed to initialize the fields. If it's more than a small amount you could put it in another class. Choose whatever makes the code easy to navigate. – Jointworm

@Jointworm thanks a lot... It would be great if you can take a look at this post detailing the question I have: https://mcmap.net/q/275376/-singleton-in-java-to-obtain-jwt-token-which-method-is-better/4180447 – Balder

Answer 2

all above methods are eagerly initializing object. how about this. This will help you to initialize ur class lazily. You may have heavy object and you don't want to initialize on startup.

public class MySinglton { 

  private MySinglton (){}

  private static volatile MySinglton s;

  public static MySinglton getInstance(){

   if (s != null ) return s;

    synchronized(MySinglton.class){

     if (s == null ) {

      s = new MySinglton();
     }
  }

  return s;

}

} 

Prophecy

I'm not sure if this article still applies but according to this it's broken: cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html – Godevil

@Godevil the volatile keyword will keep it safe :) The link you shared just go through the last section. One of the solution is make ur singleton variable volatile. – Prophecy

this implementation has a public constructor, so anyone can create as many objects as she wishes, so it cannot be considered as a singleton. You let the default public constructor to escape – Shayneshays

@MohammadAdnan you are right but it is really easy to omit volatile keyword and in 99.99% of cases it will still work. IMHO it is better to use other patterns that require less code (e.g. enum) – Mauricemauricio

We don't need synchronized on method getInstance if MySinglton constructor doing expensive task? – Incriminate

Answer 3

As mentiond in this great article here :

The best solution to this problem is [...] to use a static field

public class Singelton {

    private static final Singelton singleObject = new Singelton();

    public Singelton getInstance(){
        return singleObject;
    }
}

Gearwheel

You mean eager initialization of a static final field ! – Punak

Answer 4

No, its not thread-safe if the values returned on the pulbic methods are changeble objects.

To this class be Thread-safe one way is to change it to be immutable.

To do that, you could change this methods like this:

public Keyspace getKeyspace() {
    // make a copy to prevent external user to modified or ensure that Keyspace is immutable, in that case, you don't have to make a copy
    return new Keyspace( keyspace );
}

public ColumnFamily<String, String> getEmp_cf() {
    // Same principle here. If ColumnFamily is immutable, you don't have to make a copy. If its not, then make a copy
    return new ColumnFamily( emp_cf );
}

In this book Java Concurrency in Practice you can see the principle of that immutability.

Abamp

Answer 5

This should be the correct way to implement Singleton pattern using double checked locking principle:

class MySinglton { 

    private static volatile MySinglton instance;
    
    private MySinglton() {}

    public static MySinglton getInstance() {
        if (instance == null) {
            synchronized (MySinglton.class) {
                if (instance == null) {
                    instance = new MySinglton();
                }
            }
        }
        return instance;
    }

}

Charlottcharlotta

Answer 6

No, this does not appear to be thread-safe. It appears that you there is mutable data accessible after the call to getInstance, where the lock would have been released.

Drowse

Answer 7

I think this will do the same thing without having to check for instance every time. static is the same as check first time

public class Singl {        
    private static Singl _instance;
    //other vars        
    static{
            //synchronized(Singl.class){//do not need
                    _instance = new Singl();
            //}
    }

     public static Singl getInstance() {
             return _instance;

     }

     private Singl(){
             //initizlize
     }

}

Sinful

I didn't DV but there are few issues here. 1) static init and synchronized are useless: Java VM spec guarantees safety of simple static Singl _instance = new Singl(); 2) you didn't point out that it's eager initialization, as opposed to lazy one that is shown in question – Assentor

this wont be eager. it will happen first time you call getInstance as that is when the class is accessed the first time. or you do a class.forname. ty about static sync. – Sinful

it's eager in the sense that it's not controlled by getInstance invocations. Think of it, you may innocently refer Singl.class and boom! initialization is there, even though you didn't even attempt to invoke getInstance – Assentor

Answer 8

After java 1.5 version we can use volatile. If we used volatile java key ward, we can create singlton class with thread safe, Because instance variable share with Other thread as well.

public class SingleWithThreadSafe {

    // create an object static referance of SingleWithThreadSafe with volatile
    private static volatile SingleWithThreadSafe instance = null;

    // if we make the constructor private so that this class cannot be
    // instantiated from out side of class
    private SingleWithThreadSafe() {
    }

    // Get only object available
    public static SingleWithThreadSafe getInstance() {
        if (instance == null) {
            instance = new SingleWithThreadSafe();
        }
        return instance;
    }

    public void showMessage() {
        System.out.println("Hello World!");
    }
}

Grovel