Singleton rule is not working with static member variables

Question

I am working in singleton design pattern. I am using one static variable as member variable for my class. According to my code It is creating more than one instance even though i am I made singleton class. Please help me Is it correct way to do things? //This is my code

public class MySingleton {
    public static MySingleton instance = null;// new MySingleton();
    static int count;

    private MySingleton() {
        count++;
    }

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

        }
        return instance;
    }

    /**
     * @param args
     */
    public static void main(String[] args) {
        // TODO Auto-generated method stub

        MySingleton news = new MySingleton().getInstance();
        System.out.println(news.count);
        MySingleton news1 = new MySingleton().getInstance();

        System.out.println(news1.count);
        MySingleton news2 = new MySingleton().getInstance();
        System.out.println(news2.count);
    }

}

The best way to implement singletons is with an [enum](http://stackoverflow.com/questions/70689/what-is-an-efficient-way-to-implement-a-singleton-pattern-in-java). — GriffeyDog, Apr 23 '14 at 14:23

Mzf · Answer 1 · 2014-04-24T08:35:00.623

2

your code is not thread safe.

let say that you do 2 calls from 2 thread to MySimgleton().getInstance()

If the context switch happens after the if - then you will create 2 instances

edited Apr 24 '14 at 08:35

answered Apr 23 '14 at 13:42

Mzf

5,210
2
24
37

Reimeus · Answer 2 · 2014-04-23T14:16:58.720

1

You need to use the static method to access the class to allow the class to control access to its only instance

MySimgleton news = new MySimgleton().getInstance();

should be

MySimgleton news = MySimgleton.getInstance();

but this lazy initialization approach is not thread safe. You could do

public class MySingleton {
    private static class SingletonHolder {
        static final MySingleton INSTANCE = new MySingleton();
    }

    public static MySingleton getInstance() {
        return SingletonHolder.INSTANCE;
    }
    ...
}

edited Apr 23 '14 at 14:16

answered Apr 23 '14 at 13:41

Reimeus

158,255
15
216
276

@wheaties One of the lines is creating a new instance of `MySimgleton` while the other is accessing the `static` method `getInstance` through a qualified invocation. – Jeffrey Apr 23 '14 at 13:49
@Jeffrey Prior to your comment, both lines were exact duplicates. – wheaties Apr 23 '14 at 13:51
Why would you create a separate class to hold the INSTANCE? – Erwin Bolwidt Apr 23 '14 at 14:23
To avoid synchronization (but still make it thread safe). See [Initialization-on-demand holder idiom](http://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom) – Reimeus Apr 23 '14 at 14:36
@Reimeus why i shouldn't use new MySingleton().getInstance()? – Riding Cave Apr 23 '14 at 15:51
because you're creating separate instances for your singletons which is the opposite of what youre trying to do :) – Reimeus Apr 23 '14 at 16:09

Lei Gao · Answer 3 · 2014-04-23T14:51:19.480

In Main function, it should be:

public static void main(String[] args) {
    // TODO Auto-generated method stub

    MySimgleton news = MySimgleton.getInstance();
    System.out.println(news.count);
    MySimgleton news1 = MySimgleton.getInstance();

    System.out.println(news1.count);
    MySimgleton news2 =  MySimgleton.getInstance();
    System.out.println(news2.count);
}

For this case, it is better:

public class Test {
    public static void main(String[] args) {
        // TODO Auto-generated method stub

        MySimgleton news = MySimgleton.getInstance();
        System.out.println(news.count);
        MySimgleton news1 = MySimgleton.getInstance();

        System.out.println(news1.count);
        MySimgleton news2 = MySimgleton.getInstance();
        System.out.println(news2.count);
    }
}

class MySimgleton {
    public static MySimgleton instance = null;// new MySimgleton();
    static int count;

    private MySimgleton() {
        count++;
    }

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

        }
        return instance;
    }
}

score 0 · Answer 4 · answered Apr 23 '14 at 13:48

To avoid multi-threaded access errors, initialize the instance in the declaration. If you do want to initialize it in your getInstance method, wrap the synchronized block around the if statement and make the field instance private (and remote the final keyword)

Access the getInstance method through the class name, don't call the constructor first.

public class MySimgleton {
    // You don't need a synchronized block if you initialize it in the declaration
    public static final MySimgleton instance = new MySimgleton();
    static int count;

    private MySimgleton() {
        count++;
    }

    public static MySimgleton getInstance() {
        // Only if you didn't initialize it in the declaration itself.
        // synchronized (MySimgleton.class) {
        //     if (instance == null) {
        //         instance = new MySimgleton();
        //     }
        // }
        return instance;
    }

    public static void main(String[] args) {
        MySimgleton news = MySimgleton.getInstance();
        System.out.println(news.count);
        // etc.
    }
}

score 0 · Answer 5 · answered Apr 23 '14 at 13:49

The issue with your code is that you are calling the constructor yourself

new MySimgleton().

Even though the constructor is private you are able to call it from within the class.

You should onlt call the static getInstance Method to get instance of your object, and static mehtods can be accesed by Class directly.

MySimgleton.getInstance();

score 0 · Answer 6 · answered Apr 23 '14 at 13:50

First thing your class is perfect singleton for single threaded application,

only thing is you have one static varibale called count , static means common to all the objects you create and evrytime you create object you are incrementing count hence it gives 1,2,3 output.

but try to print object , you will see the same refernce address

i added print statemnt to print object and got as below

2
com.kb.designpatterns.MySingleton@15db9742
3
com.kb.designpatterns.MySingleton@15db9742
com.kb.designpatterns.MySingleton@15db9742
4

you are getting the same instance as instance variable is not getting reassigned again, but because class's constructor is called from main manually, multiple objects are getting created and hence count is incrementing — coder, Apr 23 '14 at 13:52

score 0 · Answer 7 · answered May 28 '14 at 22:48

As @Mzf has mentioned

your code is not thread safe

You can fix the problem by including if (instance == null) in the synchronized block.

However, in order to minimize synchronization impact in a heavily multy-threaded environment you might consider double checking:

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

Singleton rule is not working with static member variables

7 Answers7