Is this singleton pattern thread safe?

Question

I have a singleton server instance and I'm curious whether my code is thread safe. I've read about different singleton patterns, and I think the way to go generally is the double-checked locking pattern, which goes as follows:

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

This is supposedly an efficient thread-safe way of setting/getting a singleton. The easiest way to get and set a singleton, I've read, is lazy instantiation, which looks like this:

public static ClassicSingleton getInstance() {
    if(instance == null) {
        instance = new ClassicSingleton();
     }
     return instance;
}

Now what I want to know is if my variation is thread-safe. My code:

public static void startServer(int listeningPortNumber) throws IOException {
    if (server != null) {
        throw new IOException("Connection exists");
    }

    server = new Server(listeningPortNumber);
}

My code is very similar to the lazy instantiation pattern above, but I can't see how my code isn't thread-safe. Is there something I'm not seeing or is this actually valid code?

---

Reference: http://www.javaworld.com/article/2073352/core-java/simply-singleton.html

Answer 1

It's not safe.

Imagine what happens if two threads call startServer at the same time (or close enough to it):

1. Thread A checks server != null, and sees that server is null -- so it doesn't throw an exception
2. Thread B does the same
3. Thread A now instantiates new Server(listeningPortNumber);
4. Thread B does the same, and presumably bad things happen at this second instantiation

If server isn't volatile, the problem is even worse, since you don't even need the interleaving anymore -- it's possible that Thread A instantiates the new Server(...), but that the write to the server field isn't seen by Thread B for a long time (possibly forever) because it's not flushed to main memory.

But the method is racy even if server is volatile, because of the interleaving.

Answer 2

No, lazy singleton pattern is not thread safe.

If you want a thread-safe version of the Singleton pattern in Java, you should implement Bill Pugh's solution. The code is here:

public class OptimalSingleton
{
  protected OptimalSingleton()
  {
    // ...
  }

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

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

More about it on SO: Singleton pattern (Bill Pugh's solution)