Does this redeem a thread-safe Double-Checked Locking pattern?

Question

The problems with the original Double-Checked Locking pattern have been well-documented: C++ and the Perils of Double-Checked Locking. I have seen the topic come up in questions on SO fairly often.

I have come up with a version that, to me, seems to solve the race condition problem of the original pattern, but does it look ok to you?

In the code below, I assume that LOCK is a properly implemented mutex type that causes a memory barrier at it's lock/unlock, and I don't attempt to deal with the deallocation of the instance, as that's not part of the pattern.

I am aware of the other possible ways to do a singleton, but that's not what I am asking for. I am specifically asking about the pattern - is the race condition solved by having the allocation outside of the mutex lock?.

template <typename T, typename LOCK>
class Singleton
{
private:
    static T * instance_;
    static LOCK mutex;

    // private - inaccessible
    Singleton();
    Singleton(const Singleton &);
    Singleton & operator=(const Singleton &);

public:
    static T * get_instance()
    {
        if (instance_ == NULL)
        {
            T * temp = new T;
            mutex.lock();
            if (instance_ == NULL)
                instance = temp;
            else
                delete temp;
            mutex.unlock();
        }
        return instance_;
    }
};

Answer 1

No this is not safe. There is a race condition on instance_ (one thread could be reading from it (if (instance_ == NULL)) while another is writing to it (instance = temp;)), so this code has undefined behaviour.

You are asking if the single memory fence created by the lock is sufficient. From the perspective of C++11, it is not. From a non-C++11 perspective, I can't say for sure, but relying on non-atomic and non-mutex types for synchronization seems unlikely to work (in the pre-C++11 world, mutexes and atomic variables only work through compiler and processor specific hacks, and it seems foolish to rely on them to do anything more than their bare specification).

Answer 2

The problem, as has been mentioned elsewhere, is that there is a data race on accesses to instance_: the first if statement reads the value, and the later assignment to instance_ writes to that variable. With no synchronization between these two operations, the behavior is undefined. But there's an easy solution if you have C++11. Change the declaration

static T * instance_;

to

static std::atomic<T *> instance_;

Now all the accesses of instance_ are atomic, which guarantees no tearing and provides synchronization.