std::shared_ptr in one writer many reader design is thread safe?

Question

In a mutithreading server, one thread(writer) update data periodically from a database and other thread(readers) process user's request with this data.

I try to use a read/write lock to meet this request, but the performance is so bad, so it's needed to find something else.

I read from https://en.cppreference.com/w/cpp/memory/shared_ptr, it's says:

All member functions (including copy constructor and copy assignment) can be called by multiple threads on different instances of shared_ptr without additional synchronization even if these instances are copies and share ownership of the same object.

Then after some research, I use to std::shared_ptr to do it. The code is like something below.

// this class is singleton
class DataManager{
public:
    // all the reader thread use this method to get data and release shared_ptr 
    // at the end of user's request
    std::shared_ptr<Data> get_main_ptr(){
        return _main_data;
    }
private:
    // data1
    std::shared_ptr<Data> _main_data;
    // data2
    std::shared_ptr<Data> _back_data;

    // read database, write data in to _data
    void update_data(std::shared_ptr<Data> _data);

    // this function called at a separate thread every 10 min
    bool reload_data(){
        // write data in back pointer
        update_data(_back_data);

        //save the _main_data
        std::shared_ptr<Data> old_ptr = _main_data;

        //exchange pointer, reader thread hold the copy of _main_data
        _main_data = _back_data;

        // wait until reader threads release all copy of _main_data
        while(old_ptr.use_count() != 1) {
            sleep(5);
        }

        // clear the data
        old_ptr->clear();
        _back_data = old_ptr;
        return;
}

}

This method seems worked in production environment. but I'm not quite sure and don't understand the thread safe level of shared_ptr. Is there problem in this method? Or other suggestions to meet my request

Answer 1

It looks like you re-assign a shared_ptr which is shared between thread:

_main_data = _back_data;

If another thread reads or copies _main_data simultaneously it may get a corrupted copy.

Assigning to a shared_ptr is not thread-safe because shared_ptr contains two pointer members and they cannot be both updated atomically. See shared_ptr:

If multiple threads of execution access the same shared_ptr without synchronization and any of those accesses uses a non-const member function of shared_ptr then a data race will occur;

To fix that race condition the code needs to use atomic_store:

atomic_store(&_main_data, _back_data);

And the readers must do:

auto main_data = atomic_load(&_main_data);

Notes section is helpful:

These functions are typically implemented using mutexes, stored in a global hash table where the pointer value is used as the key.

To avoid data races, once a shared pointer is passed to any of these functions, it cannot be accessed non-atomically. In particular, you cannot dereference such a shared_ptr without first atomically loading it into another shared_ptr object, and then dereferencing through the second object.

The Concurrency TS offers atomic smart pointer classes atomic_shared_ptr and atomic_weak_ptr as a replacement for the use of these functions.

Since C++20: These functions were deprecated in favor of the specializations of the std::atomic template: std::atomic<std::shared_ptr> and std::atomic<std::weak_ptr>.

---

Also, you should make Data destructor do all the cleanup, so that you do not have to wait until reader threads release _main_data to clean it up manually.

---

Alternatively, you can use std::atomic and boost::intrusive_ptr to make updating the data pointer thread-safe, atomic, wait-free and leak-free.

The benefit of using boost::intrusive_ptr instead of std::shared_ptr is that the former can be thread-safely created from a plain pointer because the atomic reference count is stored inside the object.

Working example:

#include <iostream>
#include <atomic>

#include <boost/smart_ptr/intrusive_ptr.hpp>
#include <boost/smart_ptr/intrusive_ref_counter.hpp>

struct Data
    : boost::intrusive_ref_counter<Data, boost::thread_safe_counter>
{};

using DataPtr = boost::intrusive_ptr<Data>;

class DataAccessor
{
    std::atomic<Data*> data_ = 0;

public:
    ~DataAccessor() {
        DataPtr{data_.load(std::memory_order_acquire), false}; // Destroy data_.
    }

    DataPtr get_data() const {
        return DataPtr{data_.load(std::memory_order_acquire)};
    };

    void set_data(DataPtr new_data) {
        DataPtr old_data{data_.load(std::memory_order_relaxed), false}; // Destroy data_.
        data_.store(new_data.detach(), std::memory_order_release);
    }
};

int main() {
    DataAccessor da;

    DataPtr new_data{new Data};
    da.set_data(new_data);
    DataPtr old_data = da.get_data();
    std::cout << (new_data == old_data) << '\n';
}

valgrind run:

$ valgrind ./test
==21502== Memcheck, a memory error detector
==21502== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==21502== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==21502== Command: ./test
==21502== 
1
==21502== 
==21502== HEAP SUMMARY:
==21502==     in use at exit: 0 bytes in 0 blocks
==21502==   total heap usage: 4 allocs, 4 frees, 73,736 bytes allocated
==21502== 
==21502== All heap blocks were freed -- no leaks are possible
==21502== 
==21502== For counts of detected and suppressed errors, rerun with: -v
==21502== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Answer 2

You skipped this part:

If multiple threads of execution access the same shared_ptr without synchronization and any of those accesses uses a non-const member function of shared_ptr then a data race will occur

shared_ptr is just a container for your pointer. It's not thread-safe. You can make it, but it's easier to use locks.