Maybe (... yeah right) solved: Double checked locking on C++: new to a temp pointer, then assign it to instance.

Question

Hi all,

This is a follow up of this post on double checked locking. I am writing a new post because it seems that posting a follow-up on "aged" posts doesn't make it as visible/active as sending out a new post, maybe because most people do not sort posts in stackoverflow by level of activity.

To all who responded, thank you for all your input in the original post. After consulting Joe Duffy's excellent book, "Concurrent Programming on Windows", I am now thinking that I should be using the code below. It's largely identical to the code from his book, except for some variable renamings and the InterlockedXXX line. The following implementation uses:

1. volatile keyword on both the temp and "actual" pointers to protect against re-ordering from the compiler.
2. InterlockedCompareExchangePointer to protect against reordering from the CPU.

So, that 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 PREVENT_IA64_FROM_REORDERING
        _ReadBarrier();
        #endif
        return *singleton_;
    }

    virtual ~LazyInit() {
        ::DeleteCriticalSection(&cs_);
    }
private:
    CRITICAL_SECTION cs_;
    Factory factory_;
    T* volatile singleton_;
};

Answer 1

I've always used a much simpler singleton pattern.

class CSomething
{
protected:
    CSomething() {};
public:
    ~CSomething() {};
    static CSomething *Get()
    {
        static CSomething s;
        return &s;
    }
    // Rest of the class
};