Thread-safe singleton using atomic

Question

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;
}

After the unique_lock the mutex is already locked. Your explicit lock and unlock are not consistent. — 273K, Jul 22 '20 at 07:39
`unique_lock` is better because once an exception is thrown or it's out of scope, `unique_lock` would be unwinded and unlock the thread. However `std::mutex` is not. — Louis Go, Jul 22 '20 at 07:45
the mutex does nothing for thread safety, each call to `get_instance` makes a new `mtx` so its not synchronizing anything. Also, why have the atomic flag if its never checked? — kmdreko, Jul 22 '20 at 07:46
Please don't modify the question in a way that it changes the meaning fo the code. you moved the mutex out of the function. — t.niese, Jul 22 '20 at 07:48
You are checking non-atomic `myclass_st` while another thread (having a mutex locked) could modify it. You need to check `atomic_flag` instead. — n. m. could be an AI, Jul 22 '20 at 07:54
Do you have heard of [Meyers Singleton](https://www.modernescpp.com/index.php/thread-safe-initialization-of-a-singleton#h3-1-meyers-singleton)? FYI: [SO: C++ Singleton design pattern](https://stackoverflow.com/a/1008289/7478597) — Scheff's Cat, Jul 22 '20 at 07:54
@n.'pronouns'm. Thank you. I should change `myclass_st == nullptr` to `atomic_flag.load() ==1` ? — Anna, Jul 22 '20 at 07:59
It is safe if we can make `myclass_st = new myClass();` atomic I think. But how? — Anna, Jul 22 '20 at 08:02
Why do you assume that `atomic_flag = 1` would make the `myclass_st` assignment thread safe? — t.niese, Jul 22 '20 at 08:03
@t.niese The memory order is seq_cst. It makes the code before visible to other threads. — Anna, Jul 22 '20 at 08:06
Again please don't change your question continuously. That makes comments and exiting answers invalid or incomplete. — t.niese, Jul 22 '20 at 08:28
It is really not clear why you want to combine `atomic_flag` and `mtx` ? A mutex with a lock guard is already sufficient. — t.niese, Jul 22 '20 at 08:30
@Anna First part seems ok Just change `std::mutex mtx;` to `static std::mutex mtx;` and you are good to go(Its better to use lock_guard to lock mutex). — Mayur, Jul 22 '20 at 09:04
The second variant will absolutely not work. You need to check an atomic and *then* lock a mutex to initialise a non-atomic variable. — n. m. could be an AI, Jul 22 '20 at 09:23

t.niese · Answer 1 · 2020-07-22T09:49:22.577

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.

With the second example being best practice (RAII). If `new` failed or the constructor failed the first example would fail to unlock `mtx`. — Persixty, Jul 22 '20 at 08:38
Thank you. It is safe. But every thread has to get the mutex. The cost is high. — Anna, Jul 22 '20 at 08:48
@Anna that was not your initial question. As I already mentioned in my comment you continuously changed your question and its scope. Initially, you asked if the code you showed in your initial question and the first edit is thread save. — t.niese, Jul 22 '20 at 09:06
Err.. I just wan to focus on how to make the `new` atomic. Some problems not related to what I want to know. So I changed. I will change the question. Thank you. — Anna, Jul 22 '20 at 09:13

score 2 · Accepted Answer · answered Jul 22 '20 at 09:33

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;
}

Thread-safe singleton using atomic

2 Answers2