CODEWITHSUNDEEP
c++
Anna
Anna

Reputation: 261

Thread-safe singleton using atomic

Just want to know whether atomic_flag = 1; keeps the myclass_st assignment thread-safe or not. （I am sorry that my example has so many problems, so I have changed it.）

myClass* myclass_st = nullptr;
std::atomic<int> atomic_flag;
std::mutex mtx; //err
myClass* get_instance() {
    
    //std::unique_lock<std::mutex> lk(mtx);
    if (myclass_st == nullptr) {
        mtx.lock();
        if (myclass_st == nullptr) {
            myclass_st = new myClass();
            atomic_flag = 1;
            mtx.unlock(); //err
        }
    }
    return myclass_st;
}

I know we can use static after c11.

Maybe I should modify the code like this ?

myClass* myclass_st = nullptr;
std::atomic<int> atomic_flag;
myClass* get_instance() {

    if (atomic_flag.load() == 0) {
        std::unique_guard<std::mutex> lk(mtx);
        if (atomic_flag.load() == 0) {
            myclass_st = new myClass();
            atomic_flag = 1;
        }
    }
    return myclass_st;
}

Upvotes: 3

Views: 909

Answers (2)

t.niese
t.niese

Reputation: 40882

The shown code is not threaded save, because std::mutex mtx; is a new object each time. The std::mutex mtx; has to be static so that it is the same mutex for each invocation of get_instance` but even then the manual locking and unlocking of the mutex would not be valid in the current form.

EDIT after moving the std::mutex mtx; out of the function the mutex is the same for each invocation of get_instance. But it is still not threaded save. Multiple threads could pass the first if (myclass_st == nullptr) { condition where myclass_st is nullptr. If there are e.g. more then three threads that pass the first if then the thread that first calls lock will set myclass_st and release its lock on the mutex. The second thread that called lock won't release its acquired lock, so all other threads that passed the first if are blocked.

It has to be either:

myClass* get_instance() {
    mtx.lock();
    if (myclass_st == nullptr) {
      myclass_st = new myClass();
      atomic_flag = 1;
    }
    mtx.unlock();
    return myclass_st;
}

Instead of manually locking and unlocking you normally want to do the locking with a lock guard with automatic storage duration (RAII idiom), because that ensures that the lock on the mutex is always released when get_instance is left.

myClass* get_instance() {
    std::lock_guard<std::mutex> lk(mtx);
    if (myclass_st == nullptr) {
      myclass_st = new myClass();
      atomic_flag = 1;
    }
    return myclass_st;
}

EDIT No neither the first nor the second example is thread save. For the second example as you show it the if (atomic_flag.load() == 0) { /** ... **/ atomic_flag = 1;} could still be entered by two threads. So new myClass could still be done multiple times.

Upvotes: 4

KamilCuk
KamilCuk

Reputation: 141975

Just want to know whether atomic_flag = 1; keeps the myclass_st assignment thread-safe or not.

No.

Maybe I should modify the code like this ?

myClass* myclass_st = nullptr;
std::atomic<int> atomic_flag;
myClass* get_instance() {

    if (atomic_flag.load() == 0) {
        myclass_st = new myClass();
        atomic_flag = 1;
    }
    return myclass_st;
}

If you intent get_instance to be called by multiple threads, then no.

I guess you want:

myClass* myclass_st = nullptr;
std::atomic<int> atomic_flag{0};
std::mutex mtx;
myClass* get_instance() {
    if (atomic_flag == 0) {
        std::unique_lock<std::mutex> lk(mtx);
        if (myclass_st == nullptr) {
            myclass_st = new myClass();
            atomic_flag = 1;
        }
    }
    return myclass_st;
}

Upvotes: 2

Related Questions