0

I have been reading about thread safe singletons and the implementation I find everywhere has a getInstance() method something like this:

Singleton* getInstance()
{
    if ( !initialized )
    {
        lock();
        if ( !initialized )
        {
            instance = new Singleton();
            initialized = true;
        }
        unlock();
    }

    return instance;
}
  • Is this actually thread safe?
  • Have I missed something or is there a small chance this function will return an uninitialized instance because 'initialized' may be reordered and set before instance?

This article is on a slightly different topic but the top answer describes why I think the above code is not thread safe:

Why is volatile not considered useful in multithreaded C or C++ programming?

Community
  • 1
  • 1
digby280
  • 879
  • 1
  • 7
  • 22
  • 2
    No, it's not thread safe. And, well, if you use a Singleton then you get what you ask for, really. – Puppy Aug 12 '12 at 09:26
  • 1
    Or in short, not you missed something but the author of that code. However I wonder if a memory barrier between assignment of instance and assignment of initialized would fix the problem (assuming initialized is of type `volatile sig_atomic_t`). – celtschk Aug 12 '12 at 09:32
  • 1
    Consider reviewing: http://stackoverflow.com/questions/6086912/double-checked-lock-singleton-in-c11 – dans3itz Aug 12 '12 at 10:13
  • A memory barrier was my thought too. – digby280 Aug 12 '12 at 11:02

3 Answers3

4

Not a good idea. Look for double check locking. For instance:

http://www.drdobbs.com/cpp/c-and-the-perils-of-double-checked-locki/184405726

http://www.drdobbs.com/cpp/c-and-the-perils-of-double-checked-locki/184405772

AProgrammer
  • 51,233
  • 8
  • 91
  • 143
0

It is indeed not thread safe, because after the pointer gets returned you still work with it, although the mutex is unlocked again.

What you can do is making the child class which inherits from singleton, thread safe. Then you're good to go.

Alzurana
  • 23
  • 1
  • 6
0

Below is the code for a thread-safe singleton, using Double Check and temporary variable. A temporary variable is used to construct the object completely first and then assign it to pInstance.

Singleton* Singleton::instance() {
   if (pInstance == 0) {
      Lock lock;
      if (pInstance == 0) {
         Singleton* temp = new Singleton; // initialize to temp
         pInstance = temp; // assign temp to pInstance
      }
   }
   return pInstance;
}
Vishal Chaudhary
  • 176
  • 3
  • 4
  • That's the exact code from the article linked in AProgrammer's answer. The article explains why this code is not thread safe. The temp variable will be optimized away. These days we have ways to enforce memory ordering. – digby280 Jan 18 '22 at 22:16