Double-checked locking and unique_ptr static initialization in Visual C++

Question

I've seen singleton implemented using double-check locking like this:

Foo& Foo::Instance()
{
    static std::unique_ptr<Foo> instance;
    if (!instance)
    {
        boost::unique_lock<boost::mutex> lock(MUTEX);
        if (!instance)
            instance.reset(new Foo());
    }
    return *instance;
}

I know that double-checked locking is fixed in C++, but in our project we use Visual C++ 2010, which doesn't support all C++11 features.

I'm curious: in which ways is this code unsafe?

Problem 1

In Meyers-Alexandrescu paper, there is a clear example of how naive DCL may fail because of "unexpected" order of actual commands.

Singleton* Singleton::instance() {
    if (pInstance == 0) { // 1st test
        Lock lock;
        if (pInstance == 0) { // 2nd test
            pInstance = new Singleton;
        }
    }
    return pInstance;
}

Here, pInstance = new Singleton; actuall consists of 3 steps: memory allocation, Singleton constructor and assignment, and compiler may place them in different order. If assignment happens before constructor, concurrent thread may end up using uninitialized piece of memory instead of valid Singleton instance.

I wonder: does it still apply to my example, where instead of plain pointer unique_ptr is used?

From the first look, instance.reset(new Foo()); seems OK: reset is called only after Foo is fully initialized. But what if inlining occurs? I'm in doubt about thread-safety of this.

Problem 2

Another concern: is static std::unique_ptr<Foo> instance; itself safe? AFAIK, static X x; translates to something like this:

if (!x_is_initialized) {
    x_is_initialized = true;
    x = new X()
}

So in my case, unique_ptr may be allocated by thread #1 (but not constructed yet!), then thread #2 takes over, and it sees that unique_ptr is seemingly initialized and has some value (actually a bogus pointer, which has not yet been replaced by nullptr). Thread #2 happily dereferences that bogus and program crashes with access violation.

Can it actually happen?

Answer 1

Don't optimize prematurely

Unfortunately, MSVC 2010 does does not support 'magic statics' that, in effect, perform automatic double-checked locking. But before you start optimizing here: Do you REALLY need it? Don't complicate your code unless it's really necessary. Especially, since you have MSVC 2010 which does not fully support C++11 you don't have any portable standard way that guarantees proper multi-threading.

The way to get it to work

However, you can use boost::atomic<Foo*> to deal with the problem and the compiler will most likely handle the problem correctly. If you really want to be sure, check the compiled assembly code in both debug and release mode. The assignment to an atomic pointer is guaranteed to take place after the construction, even if code is inlined. This is due to special compiler intrinsics for atomic operations which are guaranteed not be be reordered with writes that are supposed to happen before the write to the atomic variable.

The following code should do the trick:

Foo & Foo::Instance()
{
    static boost::atomic<Foo *> instance; // zero-initialized, since static

    if ( !instance.load() )
    {
        boost::lock_guard<boost::mutex> lock(mutex);
        if ( !instance.load() )
        {
            // this code is guaranteed to be called at most once.
            instance = new Foo;
            std::atexit( []{ delete &Instance(); } );
        }
    }
    return *instance.load();
}

Problem 1

Your compiler might still reorder things in some optimization pass. If the compiler doesn't, then the processor might do some construction reordering. Unless you use genuine atomics with their special instructions or thread-safe constructs like mutexes and condition variables you will get races, if you access a variable through different threads at the same time and at least one of them is writing. Never EVER do that. Again, boost::atomic will do the job (most likely).

Problem 2

That is exactly what magic statics are supposed to do: They safely initialize static variables that are accessed concurrently. MSVC 2010 does not support this. Therefore, don't use it. The code that is produced by the compiler will be unsafe. What you suspected in your question can in theory really happen. By the way: The memory for static variables is reserved at program start-up and is AFAIK zero-initialized. No new operator is called to reserve the memory for the static variable.

Still a problem?

The std::atexit() function might not be thread-safely implemented in MSVC 2010 and should possibly not be used at all, or should only be used in the main() thread. Most implementations of double-checked locking ignore this clean-up problem. And it is no problem as long as the destructor of Foo does nothing important. The unfreed memory, file handles and so forth will be reclaimed by the operating system anyways. Examples of something important that could be done in the destructor are notifying another process about something or serializing state to disk that will be loaded at the next start of the application. If you are interested in double checked locking there's a really good talk Lock-Free Programming (or, Juggling Razor Blades) by Herb Sutter that covers this topic.

Answer 2

The implementation is not thread safe: there is a data race since it's possible that some thread is accessing instance at the same time that another is modifying it:

static std::unique_ptr<Foo> instance;
if (!instance)                     // Read instance
{
    boost::unique_lock<boost::mutex> lock(MUTEX);
    if (!instance)
        instance.reset(new Foo()); // Modify instance
}

Programs with data races have undefined behavior, realistically for VC++ on x86 it could result in:

• Reader threads seeing a pointer value in instance to a Foo that isn't fully constructed. The compiler is free to reorder the store into instance with the construction of the Foo, since such a reordering would not be observable in a program without data races.

• Multiple Foos being constructed simultaneously and leaked. The compiler may optimize away the check of !instance inside the lock, since it already knows the value of instance from the earlier check and that value could not have changed in a program without data races. This results in multiple threads allocating Foos and storing them in the unique_ptr. The check of the contained pointer value could be optimized out inside reset as well, resulting in leaked Foo objects.

For the question of whether or not static std::unique_ptr<foo> instance; is thread-safe in the first place, the answer is a firm maybe. std::unique_ptr's default constructor is constexpr in C++11, so instance should be zero-filled during constant initialization before main is even entered. Of course VS2010 does not support constexpr, so your guess is as good as mine. Examination of the generated assembly should give you an idea.

Answer 3

If you're OK with C++11, everything just got a whole lot simpler thanks to §6.7.4:

If control enters the declaration concurrently while the variable is being initialized, the concurrent execution shall wait for completion of the initialization

How simple? This simple:

Foo& Foo::Instance()
{
     // this is thread-safe in C++11
     static Foo instance;
     return instance;
}

To what degree VC2010 supports this, I do not know. And while I realize this doesn't literally answer your questions about your specific problems (I believe that instance.reset() solves the simple pointer problem, since reset() is basically just an assignment, but am not sure), hopefully it's helpful anyway.