Double checked locking on C++: new to a temp pointer, then assign it to instance

Question

Anything wrong with the following 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 a a 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. It's possible that it might load 16 bits of a 32-bit pointer, then be swapped out before loading the rest.

If another thread sneaks in at that point and calls Instance(), you're toasted because your foo pointer is invalid.

For true security, you will have to protect the entire test-and-set mechanism, even though that means using mutexes even after the pointer is built. In other words (and I'm assuming 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 don't want a mutex (for performance reasons, presumably), an option I've used in the past is to build all singletons before threading starts.

In other words, assuming you have that control (you may not), simply create an instance of each singleton in main before kicking off the other threads. Then don't use a mutex at all. You won't have threading problems at that point and you can just use the canonical don't-care-about-threads-at-all version:

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

And, yes, this does make your code more dangerous to people who couldn't be bothered to read your API docs 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 is introduced into the language. The following is thread safe. The language guarantees that instance is only initialized once and in a thread safe manor.

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

So its lazily evaluated. Its thread safe. Its very simple. Win/Win/Win.