How do you implement a singleton efficiently and thread-safely? [duplicate]

Question

The usual pattern for a singleton class is something like

static Foo &getInst()
{
  static Foo *inst = NULL;
  if(inst == NULL)
    inst = new Foo(...);
  return *inst;    
}

However, it's my understanding that this solution is not thread-safe, since 1) Foo's constructor might be called more than once (which may or may not matter) and 2) inst may not be fully constructed before it is returned to a different thread.

One solution is to wrap a mutex around the whole method, but then I'm paying for synchronization overhead long after I actually need it. An alternative is something like

static Foo &getInst()
{
  static Foo *inst = NULL;
  if(inst == NULL)
  {
    pthread_mutex_lock(&mutex);
    if(inst == NULL)
      inst = new Foo(...);
    pthread_mutex_unlock(&mutex);
  }
  return *inst;    
}

Is this the right way to do it, or are there any pitfalls I should be aware of? For instance, are there any static initialization order problems that might occur, i.e. is inst always guaranteed to be NULL the first time getInst is called?

Bellbella

But you don't have time to find an example and tee up a close vote? I'm fresh out at the moment. – Theolatheologian

@Theolatheologian No, the questioner obviously couldn't be bothered, so why should I? I've decided to give up downvoting and closing as dupes, as I seem to be one of the few bothering to keep crap out of SO. And do you know, laziness feels good! – Corrective

I did take time to carefully describe my problem, with snippets and a discussion of what I knew/had tried. I'm sorry I've wasted your time with "crap." :( – Bellbella

@Neil: If you don't feel like searching for dupes and closing questions, why bother pointing them out and earning bad karma? This is a pretty good thread on the subject: stackoverflow.com/questions/6915. I voted to close. – Tensive

@sbi: so did I. Scattering answers throughout thousands of question is the best way to make it hard to search through them later on. – Rapine

Answer 1

Your solution is called 'double checked locking' and the way you've written it is not threadsafe.

This Meyers/Alexandrescu paper explains why - but that paper is also widely misunderstood. It started the 'double checked locking is unsafe in C++' meme - but its actual conclusion is that double checked locking in C++ can be implemented safely, it just requires the use of memory barriers in a non-obvious place.

The paper contains pseudocode demonstrating how to use memory barriers to safely implement the DLCP, so it shouldn't be difficult for you to correct your implementation.

Seizure

if(inst == NULL) { temp = new Foo(...); inst=temp;} Doesn't that guarantee that the constructor finished before inst was assigned? I realize it can (and probably will be) optimized away, but logically that solves the problem, no? – Tracee

That doesn't help because a compliant compiler is free to reorder the assignment and construction steps as it sees fit. – Seizure

I read the paper carefully, and it seems that the recommendation is simply avoid DLCP with Singleton. You will have to volatile the heck out of the class, and add memory barriers (wouldn't that affect efficiency as well?). For practical needs, use a simple, single lock, and cache the object you get from "GetInstance". – Emissive

also just read the paper and I understood the main conclusion as: DLCP can be implemented thread safe using memory barriers, but not in a portable way (before c++11) – Mello

Answer 2

If you are using C++11, here is a right way to do this:

Foo& getInst()
{
    static Foo inst(...);
    return inst;
}

According to new standard there is no need to care about this problem any more. Object initialization will be made only by one thread, other threads will wait till it complete. Or you can use std::call_once. (more info here)

Erewhile

This is the C++11 solution I would expect people to implement. – Lotus

Sadly, this is not thread safe in VS2013, see "Magic Statics" here: msdn.microsoft.com/en-gb/library/hh567368.aspx – Caco

VS 14 seems to fix this -- blogs.msdn.com/b/vcblog/archive/2014/06/03/… – Aland

To avoid confusion, maybe you could either add static to the function declaration or explicitly state that it is a non-member function. – Scatterbrain

Are calls for this instances from different threads are thread-safe or functions of instanced class must care of atomicity by itself? – Innuendo

using std::call_once : modernescpp.com/index.php/thread-safe-initialization-of-data, and look at scott meyers singleton – Exaggerated

Answer 3

Herb Sutter talks about the double-checked locking in CppCon 2014.

Below is the code I implemented in C++11 based on that:

class Foo {
public:
    static Foo* Instance();
private:
    Foo() {}
    static atomic<Foo*> pinstance;
    static mutex m_;
};

atomic<Foo*> Foo::pinstance { nullptr };
std::mutex Foo::m_;

Foo* Foo::Instance() {
  if(pinstance == nullptr) {
    lock_guard<mutex> lock(m_);
    if(pinstance == nullptr) {
        pinstance = new Foo();
    }
  }
  return pinstance;
}

you can also check complete program here: http://ideone.com/olvK13

Excaudate

@Anterior what's your suggestion? – Excaudate

A simple static Foo foo; and return &foo; inside the instance function would be enough; static initialization is thread-safe in C++11. Prefer reference to pointers though. – Anterior

I get the error message in MSVC 2015: Severity Code Description Project File Line Source Suppression State Error (active) more than one operator "==" matches these operands: – Rocher

@Excaudate may be you can also make copy constructor, move constructor, assignment operator, move assignment operator as private. – Tajuanatak

Answer 4

Use pthread_once, which is guaranteed that the initialization function is run once atomically.

(On Mac OS X it uses a spin lock. Don't know the implementation of other platforms.)

Devotional

Answer 5

TTBOMK, the only guaranteed thread-safe way to do this without locking would be to initialize all your singletons before you ever start a thread.

Tensive

Answer 6

Your alternative is called "double-checked locking".

There could exist multi-threaded memory models in which it works, but POSIX does not guarantee one

Austria

Answer 7

ACE singleton implementation uses double-checked locking pattern for thread safety, you can refer to it if you like.

You can find source code here.

Ramsgate

Answer 8

Does TLS work here? https://en.wikipedia.org/wiki/Thread-local_storage#C_and_C++

For example,

static _thread Foo *inst = NULL;
static Foo &getInst()
{
  if(inst == NULL)
    inst = new Foo(...);
  return *inst;    
 }

But we also need a way to delete it explicitly, like

static void deleteInst() {
   if (!inst) {
     return;
   }
   delete inst;
   inst = NULL;
}

Pictish

Answer 9

The solution is not thread safe because the statement

inst = new Foo();

can be broken down into two statements by compiler:

Statement1: inst = malloc(sizeof(Foo));
Statement2: inst->Foo();

Suppose that after execution of statement 1 by one thread context switch occurs. And 2nd thread also executes the getInstance() method. Then the 2nd thread will find that the 'inst' pointer is not null. So 2nd thread will return pointer to an uninitialized object as constructor has not yet been called by the 1st thread.

Bradstreet

No, it's unsafe, period. It doesn't have to be "broken up by the compiler" to be unsafe. – Tristram