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

@StackFlowed I see. Well, I think I don't really need it to be, I'm just curious and wanted to educate myself. — Olavi Mustanoja, Oct 30 '14 at 19:19
This question seems relevant: http://stackoverflow.com/questions/5307003/java-multi-threading-atomic-reference-assignment — Alfonso, Oct 30 '14 at 19:24

score 8 · Accepted Answer · answered Oct 30 '14 at 19:19

It's not safe.

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

Thread A checks server != null, and sees that server is null -- so it doesn't throw an exception
Thread B does the same
Thread A now instantiates new Server(listeningPortNumber);
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.

score 3 · Answer 2 · edited May 23 '17 at 12:20

3

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)

edited May 23 '17 at 12:20

Community

1
1

answered Oct 30 '14 at 19:15

Nagy Vilmos

1,878
22
46

I wasn't asking whether or not lazy instantiation is thread-safe. I know it isn't. How about my variation? If it's not, can you explain why. – Olavi Mustanoja Oct 30 '14 at 19:17
Probably yeah. But more expensive the lock, the double checking, etc. than Bill Pugh's solution. So why do you want to invent a new way, which (probably) worse than the existing solutions. And no one knows, so no one will be able to understand you code easily. – Nagy Vilmos Oct 30 '14 at 19:21
1

I have no interest in a proved pattern. I'm simply curious. – Olavi Mustanoja Oct 30 '14 at 19:22
I would say, that yeah, it's thread safe. But the idea behind your solution is the same if the `getInstance()` would be `synchronized`. Except your solution would be slower, 'cause the double `if`. – Nagy Vilmos Oct 30 '14 at 19:27

score 3 · Answer 3 · answered Oct 30 '14 at 19:23

Your code is not thread safe.

Suppose you have two threads, A and B that call startServer(...), and that prior to these calls the server has not been initialized. A passes one value (a) and B passes another (b) to the method. The only thing you know is that within each thread the order of operations is defined.

So the following is possible:

A: check server != null  (false)
B: check server != null  (false)
A: server = new Server(a)
B: server = new Server(b)

Clearly this violates the singleton. Thread A will see a server that's initialized to a value different to the one passed in, and an exception was never thrown.

score 1 · Answer 4 · answered Oct 30 '14 at 19:24

Your code isn't thread safe! Suppose 2 different threads simultaneously call startServer method Both see the server variable is null so will skip the if block And each thread creating an instance of Server! So you will end with 2 instance of a server in your application.

Is this singleton pattern thread safe?

4 Answers4