safe way to get a singleton instance from multithreads

Question

Method 1

DataCenter* DataCenter::getInstance()
{
    static DataCenter instance;
    return &instance;    
}

Method 2

DataCenter* DataCenter::getInstance()
{
    if (!m_instanceFlag)
    {
        m_instance = new DataCenter();
        m_instanceFlag = true;
    }    
    return m_instance;      
}

I am working on a multi threaded programming and DataCenter will be accessed by more than one thread. I used to have method 2 to get the instance of DataCenter and it worked fine. But I noted that I need to guard the singleton instance from being called by multi threads.

My question is first do I really need to guard the singleton instance? or does OS do this for me? Second questions is that, is the first method a right way to get the singleton instance?

Thanks in advance...

Answer 1

1.You do need to guard it and even if you don't, of course, OS wouldn't do it for you. Use following code for thread-safety:

DataCenter* DataCenter::getInstance()
{
    MutexLocker locker(DataCenter::m_mutex);
    if(!m_instanceFlag)
    {
        m_instance = new DataCenter();
        m_instanceFlag = true;
    }
    return m_instance;
}

Edit:

Where MutexLocker is something like this:

class MutexLocker
{
    pthread_mutex_t &mutex;
    public:
    MutexLocker(pthread_mutex_t &mutex):mutex(mutex)
    {
        if(pthread_mutex_lock(&this->mutex)!=0)
            throw std::runtime_error("mutex locking filed");
    }
    ~MutexLocker(void)
    {
        if(pthread_mutex_unlock(&this->mutex)!=0)
            throw std::runtime_error("mutex unlocking filed");
    }
}

2.First method looks ok, but not thread-safe.

Answer 2

I suppose I might as well provide my answer...

First, "Method 1" works fine on C++0x. From the draft standard, section 6.7 (4) (emphasis mine):

The zero-initialization (8.5) of all block-scope variables with static storage duration (3.7.1) or thread storage duration (3.7.2) is performed before any other initialization takes place. ... Otherwise such a variable is initialized the first time control passes through its declaration; such a variable is considered initialized upon the completion of its initialization. ... If control enters the declaration concurrently while the variable is being initialized, the concurrent execution shall wait for completion of the initialization.

So if you have C++0x, "Method 1" is a simple, correct, and 100% portable way to implement a thread-safe singleton. (Even prior to C++0x, g++ on Unix ensures this idiom will work. I do not know about MSVC.) This is also very likely to be the fastest solution, since the compiler can see exactly what you are doing and it knows more than you about your CPU's architecture.

The easy way to fix "Method 2" is to put a mutex around the entire function, as Mihran suggests. But that might have unfortunate performance consequences, so people are always looking for ways to optimize it. Most of those ways introduce subtle bugs...

The "double-checked locking pattern" is the classic example. It looks like this:

if (m_instance == NULL) {
    grab_mutex();
    if (m_instance == NULL)
        m_instance = new Whatsit();
    release_mutex();
}

return m_instance;

There are two problems with this pattern. First, individual memory accesses are not guaranteed to be atomic in general; simultaneous loads and stores of a single memory location by different threads can result in garbage being read. (I grant this is unlikely for a pointer value -- and certainly it will not happen on x86/x86_64 -- but do you really want your code to work on just one platform for one day?)

Second, both the compiler and the CPU are free to reorder memory accesses. So the thread that runs the Whatsit constructor and then fills in m_instance might actually perform those writes out-of-order... Meaning another thread can test m_instance, observe it to be non-NULL, and then access the object before it has been initialized. This is not hypothetical; it really happens on modern CPUs. Worse, the more modern the CPU, the more likely it is to be a problem, because CPUs keep getting more and more aggressive about reordering memory accesses.

To fix this pattern, the first read of m_instance needs to have "acquire semantics", and the write to m_instance needs to have "release semantics". Definitions: If a memory load L has "acquire semantics", then subsequent loads may not be reordered to happen before L. Similarly, if a store S has "release semantics", then previous stores must not be reordered to happen after S.

Both of these are required in order for "double-checked locking" to be thread-safe. (Plus the individual loads and stores must be atomic.)

As Justin points out, declaring m_instance "volatile" will provide these semantics on MSVC. But this is not guaranteed by any standard, and in fact it is not true for many compilers (e.g. g++ on Unix). But if you are certain you will never care about any platform other than x86 Windows -- which you aren't -- then "volatile" will work.

What about using a compare-and-swap operation like InterlockedCompareExchangePointer?

if (m_instance == NULL) {
    Whatsit *p = new Whatsit();
    if (InterlockedCompareExchangePointer(&m_instance, p, NULL) != NULL)
        delete p;
}

return m_instance;

The documentation says InterlockedCompareExchangePointer provides a "full memory barrier", which means it definitely has release semantics. So is this correct?

No, not in general. The problem is that the outer read of m_instance does not necessarily have "acquire semantics". So in principle, another thread could still read the members of the object before they have been initialized. Modern CPUs do perform "speculative loads"; that is, they try to "guess" what you are going to need in order to load it from memory ahead of time. Granted, on a cache-coherent platform like x86, the CPU would have to be near-psychic to read an object before its pointer had even been computed... But if you use this pattern just because it happens to work on your system today, when it breaks on some future system and introduces night-impossible-to-debug failures, somebody will be cursing your name. So please do not do this.

Finally, you can use an API designed for one-time initialization, like the "One-Time Initialization API" on Windows or pthread_once on Unix. These work correctly by definition, but obviously they are platform-specific.

Bottom line: Use "Method 1" if your system supports it for this purpose.

Answer 3

As people mentioned in comments, the double-checked lock is not a thread safe solution. You really need to use some kind of a mechanism that will serialize access to the resource. Interlocked exchange is probably one of the simplest ways.

template <typename T>
class Singleton
{
  private:
    Singleton();
    ~Singleton();
    Singleton& operator=(const Singleton& item);

  protected:
    static volatile long m_locker;

    static T* GetPointer()
    {
      T* pTmp = NULL;
      try
      {
         static T var;
         pTmp = &var;
      }
      catch(...)
      {
         //assert(false);
         pTmp = NULL;
      }
      return pTmp;
    }

  public:
    static T* Get()
    {
       while(::InterlockedExchange(&m_locker, 1) != 0)
         ::SwitchToThread();

       T* pTmp = GetPointer();
       ::InterlockedExchange(&m_locker, 0);
       return pTmp;
    }
};

template <typename T>
  volatile long Singleton<T>::m_locker = 0;

Answer 4

You only need to guard the singleton access if calling getSingleton may also initialise the singleton -- otherwise, multiple threads may try to create it simultaneously.

A mutex is sufficient to prevent the race condition, however, each subsequent call to getSingleton must also acquire the lock, which puts a damper on performance. If this is an issue and you can deal with the additional complexity, Raymond Chen shows a way that the lock can be avoided, by allowing multiple threads to create the singleton instance and determining which to keep using interlocked operations (code inlined below):

Widget *g_pwidCached;

Widget *GetSingletonWidget()
{
 Widget *pwid = g_pwidCached;
 if (!pwid) {
  pwid = new(nothrow) Widget();
  if (pwid) {
   Widget *pwidOld = reinterpret_cast<Widget*>
       (InterlockedCompareExchangePointerRelease(
          &reinterpret_cast<PVOID&>(g_pwidCached),
          pwid, NULL));
   if (pwidOld) {
    delete pwid; // lost the race - destroy the redundant copy
    pwid = pwidOld; // use the old one
   }
  }
 }
 return pwid;
}

Of course, this is Windows-specific, but the code can be replaced with platform-specific interlocked operations without changing the meaning. (As a bonus: if you are coding on Windows, you can simply use the provided One-Time Initialization API to do the hard work for you!)

Note that the singleton's constructor must not have side-effects, otherwise you will get unexpected results. (There are more details in Raymond's full blog post.)

Answer 5

You need to use double checked lock mechanism but it may not be 100% safe as well.

DataCenter* DataCenter::getInstance()
{
  if (m_instance == null)
  {
    // some sort of synchronization lock    //1
     {  

        if (m_instance == null)             //2
            m_instance  = new DataCenter(); //3 
    }
  }
  return m_instance ;
}

A little more explanation:

Thread 1 enters the getInstance() method.

Thread 1 enters the synchronized block at //1 because instance is null.

Thread 1 is preempted by thread 2.

Thread 2 enters the getInstance() method.

Thread 2 attempts to acquire the lock at //1 because instance is still null. However, because thread 1 holds the lock, thread 2 blocks at //1.

Thread 2 is preempted by thread 1.

Thread 1 executes and because instance is still null at //2, creates a Singleton object and assigns its reference to instance.

Thread 1 exits the synchronized block and returns instance from the getInstance() method.

Thread 1 is preempted by thread 2.

Thread 2 acquires the lock at //1 and checks to see if instance is null.

Because instance is non-null, a second Singleton object is not created and the one created by thread 1 is returned.

Answer 6

Yes, you really do need to do this. If thread 1 checks the instance flag and get swapped out for thread 2, which then executes the entire getInstance() method, thread 1 will then proceed to get another instance.

This is because it's already checked the flag when it was false and will not recheck it just because of a context switch.

If there's the chance that multiple threads may call getInstance() concurrently, you need to protect the operation of checking and setting the flag as an atomic unit.

Of course, you can get away with no protection if you call getInstance from your main thread before any other threads start.

You may also want to ditch the idea of using a separate flag variable as well. You can set the instance to NULL on load and just use that:

DataCenter* DataCenter::getInstance(){
    static DataCenter *m_instance = 0;
    // begin atomic unit
    if(m_instance == 0)
        m_instance = new DataCenter();
    // end atomic unit
    return m_instance;
}