Double lock check in C ++: new for temp pointer, then assign it to an instance

Question

Is there something wrong with the next Singleton implementation?

Foo& Instance() { if (foo) { return *foo; } else { scoped_lock lock(mutex); if (foo) { return *foo; } else { // Don't do foo = new Foo; // because that line *may* be a 2-step // process comprising (not necessarily in order) // 1) allocating memory, and // 2) actually constructing foo at that mem location. // If 1) happens before 2) and another thread // checks the foo pointer just before 2) happens, that // thread will see that foo is non-null, and may assume // that it is already pointing to aa valid object. // // So, to fix the above problem, what about doing the following? Foo* p = new Foo; foo = p; // Assuming no compiler optimisation, can pointer // assignment be safely assumed to be atomic? // If so, on compilers that you know of, are there ways to // suppress optimisation for this line so that the compiler // doesn't optimise it back to foo = new Foo;? } } return *foo; } 

Answer 1

No, you cannot even assume that foo = p; is atomic. Perhaps it can load 16 bits of a 32-bit pointer, and then swap it before loading the rest.

If at this moment another thread is Instance() and calls Instance() , you are frying, because the pointer foo invalid.

For true security, you will have to protect the entire testing and tuning mechanism, although this means using mutexes even after creating the pointer. In other words (and I assume that scoped_lock() will release the lock when it goes out of scope here (I have little experience with Boost)), something like:

 Foo& Instance() { scoped_lock lock(mutex); if (foo != 0) foo = new Foo(); return *foo; } 

If you do not want to use mutexes (for performance reasons, presumably), then the option that I used in the past is to create all singletones before the threads begin.

In other words, if you have this control (you cannot), just instantiate each singleton in main before starting other threads. Then do not use the mutex at all. At this point, you will not have problems with threads, and you can simply use the canonical don't-care-about-threads-at-all version:

 Foo& Instance() { if (foo != 0) foo = new Foo(); return *foo; } 

And yes, it makes your code more dangerous for people who couldn’t bother to read your API documents, but (IMNSHO) they deserve everything they get :-)

Answer 2

Why not keep it simple?

 Foo& Instance() { scoped_lock lock(mutex); static Foo instance; return instance; } 

Edit: In C ++ 11, where threads are introduced into the language. Below is the flow information. The language ensures that the instance is initialized only once in a thread-safe manor.

 Foo& Instance() { static Foo instance; return instance; } 

So he was lazily appreciated. Its thread is safe. It is very simple. Win / Win / Win.

Answer 3

It depends on which stream library you are using. If you are using C ++ 0x, you can use atomic comparison and replace operations and write barriers to ensure that double lock checking works. If you work with POSIX threads or Windows threads, you can probably find a way to do this. The bigger question is why? It turns out that singletones are usually not needed.

Answer 4

Why don't you just use a real mutex, ensuring that only one thread tries to create foo ?

 Foo& Instance() { if (!foo) { pthread_mutex_lock(&lock); if (!foo) { Foo *p = new Foo; foo = p; } pthread_mutex_unlock(&lock); } return *foo; } 

This is a lock with verification and testing with free readers. Replace the reader-writer lock above if you want readings to be guaranteed safe in an atom-free environment.

to change . If you really want free readers, first write foo and then write the flag variable fooCreated = 1 . Checking fooCreated != 0 safe; if fooCreated != 0 , then foo initialized.

 Foo& Instance() { if (!fooCreated) { pthread_mutex_lock(&lock); if (!fooCreated) { foo = new Foo; fooCreated = 1; } pthread_mutex_unlock(&lock); } return *foo; } 

Answer 5

The new operator in C ++ always invokes a two-step process:
1.) memory allocation identical to plain malloc
2.) call the constructor for the given data type

 Foo* p = new Foo; foo = p; 

the above code will make creating a singleton in 3 steps, which is even vulnerable to the problem you are trying to solve.

Answer 6

Thanks for all your input. After consulting with Joe Duffy, an excellent book, “Parallel Programming in Windows,” I now think I should use the code below. This is pretty much the code from his book, with the exception of some renaming and the InterlockedXXX line. The following implementation uses:

• mutable keyword for both the temporary and the "actual" pointer to protect against reordering using the compiler .
• InterlockedCompareExchangePointer to protect against CPU reordering.

So this should be pretty safe (... right?):

 template <typename T> class LazyInit { public: typedef T* (*Factory)(); LazyInit(Factory f = 0) : factory_(f) , singleton_(0) { ::InitializeCriticalSection(&cs_); } T& get() { if (!singleton_) { ::EnterCriticalSection(&cs_); if (!singleton_) { T* volatile p = factory_(); // Joe uses _WriterBarrier(); then singleton_ = p; // But I thought better to make singleton_ = p atomic (as I understand, // on Windows, pointer assignments are atomic ONLY if they are aligned) // In addition, the MSDN docs say that InterlockedCompareExchangePointer // sets up a full memory barrier. ::InterlockedCompareExchangePointer((PVOID volatile*)&singleton_, p, 0); } ::LeaveCriticalSection(&cs_); } #if SUPPORT_IA64 _ReadBarrier(); #endif return *singleton_; } virtual ~LazyInit() { ::DeleteCriticalSection(&cs_); } private: CRITICAL_SECTION cs_; Factory factory_; T* volatile singleton_; }; 

Answer 7

There is nothing wrong with the code. After scoped_lock, there will be only one stream in this section, so the first incoming stream initializes foo and return, and then the second stream (if any) enters, it immediately returns, because foo is no longer zero.

EDIT: Added simplified code.

 Foo& Instance() { if (!foo) { scoped_lock lock(mutex); // only one thread can enter here if (!foo) foo = new Foo; } return *foo; }