1

I'm not confident that my code is right, and I want to confirm that the different thread use that not safe singleton method will get the difference object, but the result always print true.

behind is my code, I'm not sure the way I get instance from thread is right.

class Singleton {
    private static Singleton singleton=null;
    private Singleton(){
        System.out.println(Thread.currentThread().getName()+" Singleton init...");
    }

    public static Singleton getInstance(){
        if(singleton==null){
            singleton = new Singleton();
        }
        return singleton;
    }
}
public class SingletonDemo{
    public static void main(String[] args) throws ExecutionException, InterruptedException {

        ArrayList<Singleton> arrayList = new ArrayList<>();
        Callable<Singleton> obj = ()->{return Singleton.getInstance();};
        ExecutorService executors =  Executors.newSingleThreadExecutor();
        for (int i = 0; i < 10; i++) {
            Future<Singleton> submit = executors.submit(obj);
            arrayList.add(submit.get());
        }
        Singleton s1 = arrayList.get(0);
        for (int i = 1; i < arrayList.size(); i++) {
            System.out.println(s1==arrayList.get(i));
    }
}
jie
  • 95
  • 1
  • 8
  • 2
    Your reasoning is flawed. Not being thread safe means: it has a chance of breaking. It doesn't mean: it's guaranteed to fail. – JB Nizet Aug 12 '19 at 16:31
  • 3
    Sometimes it's hard to prove that something can break, but you're correct that the code is **not** thread safe. There's no reason Thread A and Thread B can't both do the `if` and both assign to singleton and both return, resulting in two different instances being returned (one in each thread). That's true for multiple reasons (basic thread scheduling, and separately thread-local value caching). See [this question's answers](https://stackoverflow.com/questions/70689/what-is-an-efficient-way-to-implement-a-singleton-pattern-in-java) for ways to implement singletons. – T.J. Crowder Aug 12 '19 at 16:32

1 Answers1

2

I understand that you want to demonstrate that your code is not thread-safe and it may break. That is true, the code is not thread-safe.

If there are two threads A and B that at the same time access getInstance method, both may see that its null and both may get a different instance of the class.

However, the test that you are writing to demonstrate this is not perfect, to break thread-safety we need a race condition, and a race condition occurs when two or more threads try to access shared data or resource at exactly the same time.

In your SingletonDemo you have used Executors but you are iterating and spawning one thread at a time. Since a thread is spawned sequentially there are very fewer chances that two or more thread will collide in getting the instance at the same time.

What we need is that 2 threads collide at exactly the same time, this is possible if they are started at exactly the same time (at least this is the minimum condition we can create to increase our probability of collision).

For this we need something called as CyclicBarrier, I am taking references from a SO answer https://stackoverflow.com/a/3376628/2179336

Let's rewrite our demo now:

public class SingletonDemo {
    public static void main(String[] args) throws InterruptedException, BrokenBarrierException {

    final CyclicBarrier gate = new CyclicBarrier(3);

    Thread t1 = new Thread() {
        public void run() {
            try {
                gate.await();
                Singleton.getInstance();
            } catch (InterruptedException | BrokenBarrierException e) {
                e.printStackTrace();
            }
        }
    };
    Thread t2 = new Thread() {
        public void run() {
            try {
                gate.await();
                Singleton.getInstance();
            } catch (InterruptedException | BrokenBarrierException e) {
                e.printStackTrace();
            }
        }
    };

    t1.start();
    t2.start();
    gate.await();
    }
}

Here, we are trying to span 2 threads at the same time using Cyclic Barrier.

Now let's modify Singleton class for better logging:

public class Singleton {
private static Singleton singleton = null;

private Singleton() {
}

public static Singleton getInstance() throws InterruptedException {
    System.out.println("Requesting Thread" + Thread.currentThread()
        .getName());
    if (singleton == null) {
        System.out.println("Created New Instance for " + Thread.currentThread()
            .getName());
        singleton = new Singleton();
    }
    return singleton;
    }
}

There, now try running SingletonDemo multiple times and if you are lucky, you may experience that your class is not thread-safe and get logs like this:

Requesting ThreadThread-2
Requesting ThreadThread-1
Created New Instance for Thread-1
Created New Instance for Thread-2

This tells you that instance was created for both the threads hence proving that the thread-safety was breached.

Dhawal Kapil
  • 2,584
  • 18
  • 31