C++ Guarantee that no nullptr pointer is retrieved using atomic operations

Question

It is the following piece of code

#include <memory>
#include <iostream>
#include <atomic>
class A
{
    public:
        A * get()
        {
            if(nullptr == ptr.load(std::memory_order_acquire))
            {
                ptr.store(new A(),std::memory_order_release);
            }    
            return ptr.load(std::memory_order_acquire);
        }
        inline static  std::atomic<A*> ptr = nullptr; 
};

int main()
{
    return 0;
}

Reading some documents the code above does not guaranteed that the ptr is not null when get is called . Actually the problem is when ThreadA check that ptr is null enter in CS and before call store ThreadB , enters in CS , allocation memory and returns. When threadA stores the values overwrite the previous one value. When get return the ptr is null. My question is why thread A does not see the updated value ?

The pair of calls `ptr.load()` and `ptr.store()` is not atomic. Memory leak. — 273K, May 26 '23 at 14:49
I don't think any thread can ever return `nullptr` (unless `new A()` sometimes fails and returns a nullptr instead of throwing); `ptr` never changes from non-null to null, so execution can't go past the `if` body and have the redundant later `ptr.load` read a null. The actual problem is leaking memory if two threads see `nullptr` and both allocate + store different pointers. As 273K says, it's not an atomic RMW. If you did `compare_exchange_strong(old=nullptr, tmp_object)` after a load saw a `null`, the threads that lost the race would know and could `delete` their `A` objects. — Peter Cordes, May 26 '23 at 14:50
This code has race conditions leading to memory leaks. `atomic` doesn't mean that code is thread safe out of the box. It means each single operation on it is thread safe (atomic). Basically multiple threads can reach `if` statement at the same time when `ptr` holds `nullptr`. — Marek R, May 26 '23 at 14:52
side note try to avoid naked new your code has memory leaks (even without thread safety). ptr should hold a smart pointer not a naked one. — Pepijn Kramer, May 26 '23 at 14:53

score 2 · Accepted Answer · answered May 27 '23 at 18:00

To make the posted code thread safe, you have to use a mutual exclusion mechanism for the allocation in addition to a std::atomic for the pointer. The most common choice would be locking a std::mutex. Here is what that code might look like,

class AMutex
{
public:
    AMutex * get()
    {
      if (auto p = ptr.load(); p)
          return p;

        std::lock_guard lock(mtx);
        if (not ptr.load())
            ptr.store(new AMutex());

        return ptr.load();
    }

    inline static std::atomic<AMutex*> ptr = nullptr;
    static std::mutex mtx;
};

As an aside, the code you posted looks very close to a Singleton except that the get function is not static. If a Singleton is the goal, here is a simple, thread-safe, standards-compliant option,

class A2 {
public:
    static A2 *get() {
        static A2 instance;
        return &instance;
    };
};

C++ Guarantee that no nullptr pointer is retrieved using atomic operations

1 Answers1