Why this is NOT thread safe?

Question

Hi In this code, why has been written "Not thread safe"? Thank you

class Singleton
{
  private static Singleton _instance;

  // Constructor is 'protected'
  protected Singleton()
  {
  }

  public static Singleton Instance()
  {
    // Uses lazy initialization.
    // **Note: this is not thread safe.**
    if (_instance == null)
    {
      _instance = new Singleton();
    }

    return _instance;
  }
}

Answer 1

If two threads run the if (_instance == null) check at the same time while there's no singleton instance created, they will both try to invoke new to create the singleton instance and store the references to them into the same variable.

Since the reference they will try to store to will be shared between threads this action will not be thread-safe. Also it might happen that creating two instances of the singletone class will break the program.

Answer 2

Because the Singleton doesn't provide mutual exclusion over the _instance property.

Use a lock to achieve the thread-safety:

Object thisLock = new Object();

public static Singleton Instance()
{
    lock (thisLock)
    {
        if (_instance == null)
        {
           _instance = new Singleton();
        }
    }

    return _instance;
}

This example is C# - i don't know which programming language you are using.

http://msdn.microsoft.com/en-us/library/c5kehkcz(VS.90).aspx

Answer 3

Because it is possible that multiple instances of the singleton gets created in this case. Assume that two threads have entered the Instance method and singleton object is not yet created (i.e._instance is NULL). Then assume the first thread executes the if condition and enters it. But before it does a new thread context switch happens and the second thread starts executing. It also tests the if condition and finds that it is NULL and does a new. Now first thread starts executing and creates one more instance of the object.

Answer 4

Based on RPM1984's answer:

I use the following for the lock object: object thisLock = typeof( Sinlgeton );

or just

...
lock( typeof( Singleton ) )
{
   ...
}

for the performance savy amongst you:

public Singleton getInstance()
{
    // the first query may save a performance-wise expensive lock - operation
    if ( null == _instance )
    {
       lock ( typeof( Singleton ) )
       {
          if ( null == _instance )
          {
             _ instance = new Singleton()
          }
       }
    }

    return _instance;
}

BTW: This is called the double-locked singleton pattern.

Answer 5

I think in Java it is sufficiant to just add a synchronized flag to the getInstance method. This will prevent other threads going into the method while one other is inside.

Answer 6

The biggest issue is that the check that an instance doesn't already exist and its lazy initialization is not an atomic operation.

The simplest example is that Thread A checks the conditional then yields to Thread B; Thread B checks the same conditional and creates a new object and returns that new object; Thread B then yields back to Thread A, which picks up where it left off; Thread A then goes and creates a new object and returns it.

There are some other notable problems:

1. The _singleton variable should be marked volatile to ensure that writes are properly published.
2. The class should be marked final or sealed to prevent issues with sub-classes.
3. In Java you should override the clone() method to throw an UnsupportedOperationException.
4. If for some reason there's a requirement to make the class serializable you would also need to provide an implementation to handle that scenario (otherwise a client could continuously deserialize a stream to create multiple instances).

Answer 7

If the singleton is cheap to create, and the main requirement is that everything in the application see the same one, another approach is:

  If TheSingleton Is Nothing Then
    Dim newSingleton As New Singleton
    If Interlocked.CompareExchange(TheSingleton, newSingleton, Nothing) IsNot Nothing Then
      newSingleton.Dispose  ' If applicable
    End If
  End If

If two threads pass the "If" test before either does the CompareExchange, two singletons will be created. Only the first singleton to be passed to the CompareExchange, however, will ever be used for anything; the other will be discarded.

Answer 8

Actually, the example given might be typesafe. Further, it might even be a reasonably good idea.

If multiple threads hit the null check before the first thread has written to _instance (and indeed, perhaps after the first thread has written to _instance, but before the second thread's CPU has got the new value loaded into its cache), then the second (and third, and fourth...) thread will create a new instance and write this to _instance.

In a garbage-collected language, this just means that for a brief while multiple threads will have their own version of _instance, but soon enough they'll drop out of scope and be eligible for garbage collection.

Now, this is wasteful, but whether its actually a problem or not depends on how expensive creating a new instance is and whether there are any negative consequences in having more than one in existence. Very often the downside of such needless duplication of instance objects is pretty slight, in which case it might be less of a negative than the cost of locking (locking is relatively cheap but not free, waiting for a lock can be quite expensive, and in some cases [deadlock being the most extreme case] cripplingly expensive), or even of CASsing. Even if it is more expensive, it may still not actually be unsafe.

Since we can't judge whether this is the case in the example above, we don't actually know if it's thread-safe or not.

Most likely, creating upon static construction is the way to go, though.