Is this the wrong way of initializing Singleton classes in Java?

Question

I had my singleton class initially like this

 private static MqttHandler instance = new MqttHandler();
 private MqttHandler(){};
 public MqttHandler getInstance(){
    return instance;
 }

Now in one phone it was working as expected, but in another, seems like it was creating many instances, since whenever I tried to log something, it did log multiple times. I have no idea why. The second time I tried using this

  private MqttHandler instance;
  private MqttHandler(){};
  public MqttHandler getInstance(){
      if(instance==null) instance == new MqttHandler();
      return instance;
  }

Seems to be working, at least for now, not sure if its going to crash later, Does this mean that, in my first method, whenever I returned instance, it was calling

 new MqttHandler();

thus creating new instances all the time? Why would it work on one device correctly, and then refuse completely on a different one?

The second example is acutally discouraged since it is not thread-safe. The first attempt should work fine. — Turing85, May 18 '16 at 15:51
how do you use the first one? I see you have a private constructor, meaning other classes cannot instantiate it, but then the `getInstance` method is an instance method. Also, here `if(instance==null) instance == new MqttHandler();` what are you trying to do? If it's null and you are trying to store a new instance to the `instance` variable, then do `instance = new MqttHandler();`. — Rahul Sharma, May 18 '16 at 15:56

score 0 · Answer 1 · answered May 18 '16 at 15:57

0

Both should have the same effect. Of course the latter has the benefit that the object isn't created until actually needed, but be aware that the object could be created multiple times due to concurrent calls to getInstance.

Also, you need a "static" declaration in getInstance (otherwise you would have to create an instance to get the singleton).

answered May 18 '16 at 15:57

John Coker

163
6

The second example (aka. lazy initialization) is not thread-safe. Two threads, executing `getInstance()` could see `instance` as `null` and thus initialize a new object each, breaking the Singleton-pattern. – Turing85 May 18 '16 at 16:00

Nicolas Filotto · Accepted Answer · 2016-05-18T16:25:35.590

This is how to implement a simple singleton:

 // It must be static and final to prevent later modification
 private static final MqttHandler INSTANCE = new MqttHandler();
 // The constructor must be private to prevent external instantiation  
 private MqttHandler(){};
 // The public static method allowing to get the instance
 public static MqttHandler getInstance() {
     return INSTANCE;
 }

Your second approach will work only on single threaded application because if you call getInstance at the same time with several concurrent threads, it will create several instances of your object such that the contract of your singleton would be broken. So if you need to properly lazy create your singleton, here is how to proceed:

 // The constructor must be private to prevent external instantiation   
 private MqttHandler(){};
 // The public static method allowing to get the instance
 public static MqttHandler getInstance() {
     return MqttHandlerHolder.INSTANCE;
 }
     /** 
 * The static inner class responsible for creating your instance only on demand, 
 * because the static fields of a class are only initialized when the class
 * is explicitly called, this rule is also applicable to inner static class
 * So here INSTANCE will be created only when MqttHandlerHolder.INSTANCE will be called
 */
 private static class MqttHandlerHolder {
     private static final MqttHandler INSTANCE = new MqttHandler();
 }

Some extra bits: this is lazy initialization since static fields of classes get initialized the first time, the class is used at execution time. — Turing85, May 18 '16 at 16:06
@Turing85 that's right, I added it here and in my response http://stackoverflow.com/a/37305099/1997376 — Nicolas Filotto, May 18 '16 at 16:26
@david no need as a class initialization managed by the JVM is already synchronized, once the singleton set as it is declared final, any thread is free to access it without any additional synchronization mechanism doing so would add useless contention — Nicolas Filotto, May 10 '20 at 04:40

Is this the wrong way of initializing Singleton classes in Java?

2 Answers2