CODEWITHSUNDEEP
c++multithreadingc++11thread-safety
graywolf
graywolf

Reputation: 7470

Is following singleton implementation thread safe?

Is following singleton implementation thread safe? The ::Instance method should be correct, the Dispose is my own creation so want to make sure I didn't overlooked anything.

std::atomic<S *> S::_instance;
std::mutex S::_singleton_mutex;

S& S::Instance()
{
    using namespace std;
    S * tmp = _instance.load(memory_order_relaxed);
    atomic_thread_fence(memory_order_acquire);
    if (tmp == nullptr)
    {
        lock_guard<mutex> l(_singleton_mutex);
        tmp = _instance.load(memory_order_relaxed);
        if (tmp == nullptr)
        {
            tmp = new S();
            atomic_thread_fence(memory_order_release);
            _instance.store(tmp, memory_order_relaxed);
    }
    return *tmp;
}

void S::Dispose()
{
    using namespace std;
    S * tmp = _instance.load(memory_order_relaxed);
    atomic_thread_fence(memory_order_acquire);
    if (tmp != nullptr)
    {
        lock_guard<mutex> l(_singleton_mutex);
        tmp = _instance.load(memory_order_relaxed);
        if (tmp != nullptr)
        {
            atomic_thread_fence(memory_order_release);
            _instance.store(nullptr, memory_order_relaxed);
            delete tmp;
        }
    }
}

Upvotes: 4

Views: 333

Answers (2)

kubanrob
kubanrob

Reputation: 160

As said in the other answers, the implementation seems fine. However, there is a conceptual problem: a race condition between a user and a disposer of the instance.

Thread A: var i = s::Instance();
Thread B: s::Dispose();
Thread A: i.doSth();

There might be use-cases where you can guarantee this never happens; otherwise, reference counting might be a solution to this problem.

Upvotes: 1

cdonat
cdonat

Reputation: 2822

The solution is: Yes, looks good.

More Info:

When it is acceptable for you to potentially have two instances for a short moment, where the second one will be destroyed immediately, you can get rid of the mutex:

std::atomic<S *> S::_instance;

S& S::Instance()
{
    using namespace std;
    auto tmp = _instance.load(memory_order_relaxed);
    if (tmp == nullptr)
    {
        auto tmp2 = new S();
        if( !_instance.compare_exchange_strong(tmp, tmp2) )
            delete tmp2;
    }
    return *tmp;
}

void S::Dispose()
{
    using namespace std;
    auto tmp = _instance.load(memory_order_relaxed);
    if (tmp != nullptr)
    {
        if( _instance.compare_exchange_strong(tmp, nullptr) )
            delete tmp;
    }
}

When two thread start Instance() at the same time, both see a nullptr and create a new S. Only one of them will successfuly replace the instance pointer, while the other one will immediately delete the new S instead.

Anyway, you might prefer to use Scott Meyers singleton, though it does not provide a way to dispose the object:

S& S::Instance() 
{
    // This instance will be created at first call - thread safe.
    // it lives until the program terminates.
    static Singleton instance;
    return instance;
}

This is most elegant, minimal code, and thread safe, btw.

Upvotes: 2

Related Questions