SingleTon class is not synchronized and returning multiple instances

Question

There is something wrong with my code below. I am getting multiple instances of the same singleton class when calling it from multiple threads from the same process. I am expecting the program should print "Creating instance" only once but it is printing 3 times(i.e. per thread).

package SingleTon;

class SingleTon implements Runnable {
    String name;
    int salary;
    private static SingleTon singleTonInstance;

    SingleTon() {

    }

    public static SingleTon getInstance() {
        if (singleTonInstance == null) {
            synchronized (SingleTon.class) {
                System.out.println("Creating instance");
                singleTonInstance = new SingleTon();
            }
        }
        return singleTonInstance;
    }

    @Override
    public void run() {
        System.out.format("Thread %s is starting\n",Thread.currentThread().getName());
        getInstance();
    }

}

package SingleTon;

public class SingleTonDemo {

    public static void main(String[] args) {
        System.out.println("test");
        SingleTon t = new SingleTon();
        Thread t1 = new Thread(t);
        Thread t2 = new Thread(t);
        Thread t3 = new Thread(t);
        t1.start();
        t2.start();
        t3.start();
    }

}

Output:

test
Thread Thread-0 is starting
Thread Thread-2 is starting
Thread Thread-1 is starting
Creating instance
Creating instance
Creating instance

Answer 1

You need to change the following:

1) Make your default constructor private. As written, it is package-private and can be called by other classes in the same package.

private SingleTon() {
}

Once you have done this, your SingleTonDemo class will no longer be able to call the constructor and will be forced to use the getInstance() method. Currently, each call to the constructor is creating a new instance.

2) Synchronize the entire getInstance() method. As written, 2 separate threads can get a null value for singleTonInstance, and thus each thread would create a new instance.

    public static SingleTon getInstance() {

2a) An alternate approach to ensuring there is only one instance of the class is to use static initialization. In the declaration of the static variable, you create the object:

private static SingleTon singleTonInstance = new SingleTon();

The JVM will ensure that this is called only one. That also improves your getInstance method as you would change it to:

public static SingleTon getInstance() {
    return singleTonInstance;
}

The advantage of this approach is that not only is the getInstance method simpler, it also does not incur the overhead of being synchronized.

Answer 2

You are actually creating an instance of your SingleTon class outside of the class. This should not be done when using the singleton pattern, as only a single instance of the class should be able to be created.

For the synchronization issue:

If all three threads enter the if statement in your getInstance method, every thread will create an instance (as the if statement itself is not covered by the synchronized block). One way to solve this is to make the getInstance method synchronized, i.e. adding the synchronized modifier. In that case, the fetch-and-create-if-not-exists logic in your getInstance method is executed as an atomic block.

Answer 3

The Answer by EJK looks correct.

Here is an alternative, simpler approach entirely.

Enum

Generally speaking, the best way to implement a Singleton in Java is making an enum. The enum facility in Java is much more powerful and flexible than in most platforms; see Tutorial.

An enum in Java is actually a subclass of Enum, with the compiler/JVM handling that behind the curtains. When your enum class is loaded, the JVM automatically instantiates an object for each of your enum names. In the case of a singleton, of course, we have only one such enum name. Commonly this one name is INSTANCE but can be anything you desire.

Because an enum in Java is actually a class, you can define a constructor and pass objects to that constructor. In our example below, we pass an initial value for the name and salary members you used in your Question.

Similarly, as a class, an enum in Java can have methods. In our example below, we added a method run to implement Runnable, just as you did in your Question.

In the example here, we added a UUID value as an identifier as another way for you to see that indeed we have only a single instance of our singleton.

package work.basil.example;

import java.time.Instant;
import java.util.UUID;

public enum SingletonDemo implements Runnable {
    INSTANCE( "Alice" , 10_000 );

    String name;
    Integer salary;
    UUID uuid;

    SingletonDemo ( String name , Integer salary ) {
        System.out.println( "DEBUG - Constructing `SingletonDemo` enum at " + Instant.now() );
        this.name = name;
        this.salary = salary;
        this.uuid = UUID.randomUUID();
    }

    @Override
    public void run () {
        // CAUTION: Be sure you make this method thread-safe!
        System.out.format( "Thread %s is starting. uuid: %s\n" , Thread.currentThread().getName() ,this.uuid.toString() ); // Calling getters as I did here may not be thread-safe without further precautions (synchronization, etc.).
    }

    public static void main ( String[] args ) {
        Thread t1 = new Thread( SingletonDemo.INSTANCE );
        Thread t2 = new Thread( SingletonDemo.INSTANCE );
        Thread t3 = new Thread( SingletonDemo.INSTANCE );
        t1.start();
        t2.start();
        t3.start();
    }
}

When run.

DEBUG - Constructing SingletonDemo enum at 2019-01-30T00:48:46.614462Z

Thread Thread-0 is starting. uuid: a2825344-fb15-45d1-a6e4-239fc3bdf3c5

Thread Thread-2 is starting. uuid: a2825344-fb15-45d1-a6e4-239fc3bdf3c5

Thread Thread-1 is starting. uuid: a2825344-fb15-45d1-a6e4-239fc3bdf3c5

Answer 4

public class SingleTonDemo {

public static void main(String[] args) {
    System.out.println("test");
    SingleTon t = SingleTon.getInstance();
    Thread t1 = new Thread(t);
    Thread t2 = new Thread(t);
    Thread t3 = new Thread(t);
    t1.start();
    t2.start();
    t3.start();
}

}

private SingleTon() {

}

Answer 5

First your singleton constructor MUST BE private. Second, use an object as a mutex to prevent collisions.

public class RunnableSingleton implements Runnable
{
    private static volatile RunnableSingleton instance;
    private static final Object mutex = new Object();

    private RunnableSingleton() {}

    public static RunnableSingleton getInstance()
    {
        RunnableSingleton result = instance;
        if (result == null) {
            synchronized (mutex) {
                result = instance;
                if (result == null) {
                    System.out.println("Creating instance");
                    instance = result = new RunnableSingleton();
                }
            }
        }

        return result;
    }

    @Override
    public void run()
    {
        System.out.format("Thread %s is starting\n", Thread.currentThread().getName());
        getInstance();
    }
}

Now, call the getInstance() method in your main like this:

public class SingletonDemo
{
    public static void main(String[] args) {
        System.out.println("test");
        RunnableSingleton t = RunnableSingleton.getInstance();
        Thread t1 = new Thread(t);
        Thread t2 = new Thread(t);
        Thread t3 = new Thread(t);
        t1.start();
        t2.start();
        t3.start();
    }
}

This is the output of the implementation:

test
Creating instance
Thread Thread-0 is starting
Thread Thread-2 is starting
Thread Thread-1 is starting