How can I make a public static unsynchronized getInstance() method return multiple instances of a private static reference variable to an object?

Question

One of the SCJP practice exam questions I ran across supplied the code in the SafeDeposit class. The answer to the question claimed that if another class used multiple threads that it would be possible for the unsynchronized (non thread safe) getInstance() method to return multiple instances of SafeDeposit. I have tried, and tried and cannot get the toString() method to indicate that there is ever more than one SafeDeposit instance created. Am I missing something, or is this just one of those things that "could" happen but is really, really, really unlikely to happen?

class SafeDeposit {
    private static SafeDeposit sd;
    public static SafeDeposit getInstance() {
        if(sd == null) sd = new SafeDeposit();
        return sd;
    }
    private SafeDeposit() { }
}

public class PrivCon {
    public static void main(String[] args) {
        String checker;
        SafeThief wizard = new SafeThief();
        SafeThief wizard2 = new SafeThief();
        for(int i = 0; i < 10; i ++) {
            new Thread(wizard).start();
            new Thread(wizard2).start();
        }
    }
}

class SafeThief implements Runnable {
    public void run() {
        System.out.println(SafeDeposit.getInstance().toString());
    }
}

Answer 1

is this just one of those things that "could" happen but is really, really, really unlikely to happen?

Try this code and see how unlikely it really is:

class SafeDeposit {
  private static SafeDeposit sd;
  public static SafeDeposit getInstance() {
    if(sd == null) sd = new SafeDeposit();
    return sd;
  }
  private SafeDeposit() { }

  static void warmup() {
    for (int i = 0; i < 100_000; i++) getInstance();
    sd = null;
  }
}

public class PrivCon {
  public static void main(String[] args) {
    SafeDeposit.warmup();
    SafeThief wizard = new SafeThief();
    for(int i = 0; i < 10; i ++) new Thread(wizard).start();
  }
}

class SafeThief implements Runnable {
  public void run() {
    try { Thread.sleep(100); } catch (InterruptedException e) {  }
    System.out.println(SafeDeposit.getInstance().toString());
  }
}

This is my typical output:

test.SafeDeposit@52e5376a
test.SafeDeposit@34780af5
test.SafeDeposit@351775bc
test.SafeDeposit@2b1be57f
test.SafeDeposit@6ae6235d
test.SafeDeposit@6276e1db
test.SafeDeposit@52e5376a
test.SafeDeposit@302b2c81
test.SafeDeposit@60f00e0f
test.SafeDeposit@1732a4df

Hardly any duplicates at all.

If you want to know why, it's because I added warmup code, which caused the getInstance() method to be JIT-compiled into an aggressively optimized piece of code which leverages the liberties given by the Java Memory Model.

I also added some sleep time to the beginning of the Runnable because as soon as one thread writes the value, those threads which start after that point will reliably observe the write. So it is better to first let all threads start, then let them call getInstance.

Answer 2

The way to stress a singleton is to use a CountDownLatch to make a horde of threads descend on it all at once. Sadly this code fails to print anything other than 1 but I suspect that is because I am testing it on a one-core laptop. Would someone test it on a multicore CPU and see if it prints anything else?

See comments below for tests results returning result > 1 meaning that more than one instance of the supposed singleton was actually created.

public class Test {

    static class SafeDeposit {

        private static SafeDeposit sd;

        public static SafeDeposit getInstance() {
            if (sd == null) {
                sd = new SafeDeposit();
            }
            return sd;
        }

        private SafeDeposit() {
        }
    }

    static final Set<SafeDeposit> deposits = Collections.newSetFromMap(new ConcurrentHashMap<SafeDeposit,Boolean>());

    static class Gun implements Runnable {
        private final CountDownLatch wait;

        public Gun  (CountDownLatch wait) {
            this.wait = wait;
        }

        @Override
        public void run() {
            try {
                // One more thread here and ready.
                wait.countDown();
                // Wait for the starting pistol.
                wait.await();
                // Grab an instance - nnnnnnnnow!!!.
                SafeDeposit safe = SafeDeposit.getInstance();
                // Store it in the Set.
                deposits.add(safe);
            } catch (InterruptedException ex) {
                Logger.getLogger(Test.class.getName()).log(Level.SEVERE, null, ex);
            }

        }
    }

    // Use that many Threads
    private static final int ArmySize = 1000;

    public static void main(String[] args) throws InterruptedException {
        // The Latch will wait for all threads to be ready.
        CountDownLatch latch = new CountDownLatch(ArmySize);
        Thread[] threads = new Thread[ArmySize];
        for ( int i = 0; i < ArmySize; i++ ) {
            // Make all threads and start them.
            threads[i] = new Thread(new Gun(latch));
            threads[i].start();
        }
        // Wait for all to complete.
        for ( int i = 0; i < ArmySize; i++ ) {
            threads[i].join();
        }
        // How many unique Safes did we et?
        System.out.println(deposits.size());
    }
}

Answer 3

Correct. This is NOT thread safe,

if(sd == null)              // Thread B here <---
  sd = new SafeDeposit();   // Thread A here <---
return sd;

So if you have Thread A and B as above you will get two instances of your Singleton instantiated. To see it, add a print method in the constructor like this =

private SafeDeposit() {
  System.out.println("In SafeDeposit constructor - Should only print ONCE");
  try {
    Thread.sleep(2000);     // <-- Added to help reproduce multiple 
                            //     instances being created.
  } catch (Exception e) {
  }
}

Answer 4

SafeDeposit constructor is running atomically in your code and you're not seeing the problem. To simulate a more real situation, change SafeDeposit constructor to the code below and you will see the result by yourself.

    private SafeDeposit() {
        try {
            Thread.sleep(5000);
        }
        catch (InterruptedException e) {}
    }