Is implementation of double checked singleton thread-safe?

Question

I know that the common implementation of thread-safe singleton looks like this:

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;
}

But why they say that it is a thread-safe implementation?
For example, the first thread can pass both tests on pInstance == 0, create new Singleton and assign it to the temp pointer and then start assignment pInstance = temp (as far as I know, the pointer assignment operation is not atomic).
At the same time the second thread tests the first pInstance == 0, where pInstance is assigned only half. It's not nullptr but not a valid pointer too, which then returned from the function. Can such a situation happen? I didn't find the answer anywhere and seems that it is a quite correct implementation and I don't understand anything

http://stackoverflow.com/questions/8102125/is-local-static-variable-initialization-thread-safe-in-c11 — The Techel, Apr 08 '17 at 10:43
Since C++11, the common implementation of thread safe singleton looks like `static Singleton s; return s;` — eerorika, Apr 08 '17 at 10:46
The simplest solution is to explicitly create all your Singletons in the main thread before other threads are started, because the lazy initialisation achieved by more complex solutions is usually over-engineering anyway. Or just don't use Singletons. — Christian Hackl, Apr 08 '17 at 11:31

score 5 · Accepted Answer · edited May 23 '17 at 12:02

5

It's not safe by C++ concurrency rules, since the first read of pInstance is not protected by a lock or something similar and thus does not synchronise correctly with the write (which is protected). There is therefore a data race and thus Undefined Behaviour. One of the possible results of this UB is precisely what you've identified: the first check reading a garbage value of pInstance which is just being written by a different thread.

The common explanation is that it saves on acquiring the lock (a potentially time-expensive operation) in the more common case (pInstance already valid). However, it's not safe.

Since C++11 and beyond guarantees initialisation of function-scope static variables happens only once and is thread-safe, the best way to create a singleton in C++ is to have a static local in the function:

Singleton& Singleton::instance() {
   static Singleton s;
   return s;
}

Note that there's no need for either dynamic allocation or a pointer return type.

As Voo mentioned in comments, the above assumes pInstance is a raw pointer. If it was std::atomic<Singleton*> instead, the code would work just fine as intended. Of course, it's then a question whether an atomic read is that much slower than obtaining the lock, which should be answered via profiling. Still, it would be a rather pointless exercise, since the static local variables is better in all but very obscure cases.

edited May 23 '17 at 12:02

Community

1
1

answered Apr 08 '17 at 10:49

Angew is no longer proud of SO

167,307
17
350
455

2

If pInstance is declared as std::atomic it's perfectly safe but still a bad way to do it. – Voo Apr 08 '17 at 11:06
1

@Voo, you'd still have to pay attention to the `order` argument or you risk to see the correct value for `pInstance` but not for the object it points to. – AProgrammer Apr 08 '17 at 11:10
@AProgrammer Huh? The given code doesn't specify any order, so uses sequential consistency, how would that ever cause troubles? – Voo Apr 08 '17 at 11:13
Note that some hardware/C++ implmentations give atomic reads/writes of aligned pointers, and that static pointers are aligned. However, unless you have an explicit guarantee from the compiler, you cannot trust later versions of it. – Yakk - Adam Nevraumont Apr 08 '17 at 11:35
1

This solution has potential order-of-destruction issues if some other object with static storage needs to access the Singleton in its destructor (that other object might be another Singleton). While that's not a terribly common problem in my experience, it can be eliminated quite easily by allocating the Singleton dynamically and never deleting it, which guarantees that the Singleton will live as long as the program. – Christian Hackl Apr 08 '17 at 11:37
@ChristianHackl Of course. However, "do not destroy" has its own set of issues when the destructor does something non-trivial (such as release a resource not reclaimed automatically), or when running in an environment where memory is not automatically reclaimed (which is technically a subset of the first case). – Angew is no longer proud of SO Apr 08 '17 at 11:44
@Angew: Yes, it's not ideal in either case. – Christian Hackl Apr 08 '17 at 11:56
@Yakk Atomic read/write is not essential to this, you also have to make visibility guarantees, which no sane compiler would make by default for everything due to the cost. – Voo Apr 08 '17 at 17:06
@Voo Visibility guarantees only have to be "if the pointer is modified, the pointed-to data has been created". I guess that is a reasonable fear -- temp is (rightly) eliminated, and `pInstance = new Singleton;` first assigns the address of the Singleton to pInstance, and then constructs the singleton. – Yakk - Adam Nevraumont Apr 08 '17 at 17:18
@Yakk What you need would be acquire/release semantics for every read and write that's visible to other threads. This is rather expensive and forbids a whole lot of useful optimizations. – Voo Apr 08 '17 at 17:29

Is implementation of double checked singleton thread-safe?

1 Answers1

Linked