Is this Singleton Implementation Thread-Safe?

Question

I've read the question and answers here, and learned that a true thread-safe singleton can be implemented with correctly placed memory barrier, which is stated here in chapter 6. I've come up with my own version of singleton implementation, by reviewing the code I've seen no memory ordering problem so far, but it differs a lot from a typical DCLP approach. Do you think this is a reliable implementation?

static std::atomic<T*> _instance;
...
Singleton* get_instance() {
    ins = _instance.load(std::memory_order_acquire);
    if (ins == nullptr) {
        T* temp = nullptr;
        {            
            std::lock_guard<std::mutex> guard(_lock);
            temp = new(std::nothrow) T;
        }

        if(!_instance.compare_exchange_strong(nullptr, temp, std::memory_order_release)) {
            delete temp;
        }
    }
    return _instance.load(std::memory_order_acquire);
}

This looks unsafe for me when the constructor of `T` is not thread-safe. — MikeCAT, Feb 08 '21 at 12:18
Any specific reason why `_instance` can't simply be a static member of this function, and simply letting the compiler worry about the whole thing being thread-safe? — Sam Varshavchik, Feb 08 '21 at 12:18

score 1 · Accepted Answer · answered Feb 08 '21 at 12:32

1

It is hard to beat a simple

class Singleton { ... };

Singleton* get_instance()
{
    static Singleton instance; // guaranteed to be thread safe in C++11
    return &instance;
}

Any access to the pointer after it has been returned is still not thread safe, but neither is access in your implementation.

answered Feb 08 '21 at 12:32

Mestkon

3,532
7
18

I think this is the best solution under C++11 standard. – SluggishNewb Feb 09 '21 at 02:34

Igor G · Answer 2 · 2021-02-08T12:58:03.620

Do you think this is a reliable implementation?

That depends on class T. Some classes must be singleton because there can be only one instance at any given moment, physically. I.e. an attempt to create a second instance would certainly fail, for example due to both of them requiring exclusive access to some unique external resource.

Proposed implementation does allow for concurrent construction of two instances of T, which may lead to wonderful race-induced bugs.

Edit: Even with the lock protecting new T, this solution still does not guarantee that T is constructed only once.

score 0 · Answer 3 · answered Feb 09 '21 at 19:33

Your implementation should only be used as an educational tool (use static in real software).
It has issues; multiple objects can be created (and all but one are deleted again), but at least it is thread-safe.

The first load can even be mo_relaxed since synchronization with compare_exchange(release) is covered by the load(mo_acquire) return statement.

In a DCLP implemention you would use a mutex to avoid creation of multiple objects and to block threads that are waiting for the creator to finish. Since your implementation already allows multiple objects, the mutex is not necessary from a correctness point of view (unless indeed the constructor is not thread safe).

If multiple threads create object T, the one that stores the pointer to _instance, releases the object (say thread N), while the others (say N+1) have to perform a load again; this second load(mo_acquire) in N+1 can never return nullptr. The compare_exchange in thread N+1 failed because it came later in the modification order than the first execution in thread N, which means that the compare_exchange (in N) also precedes the second load (in N+1) in the modification order and therefore it is guaranteed to return the object pointer to a synchronized object

Is this Singleton Implementation Thread-Safe?

3 Answers3