CODEWITHSUNDEEP
c++multithreadingsingletonatomicsmart-pointers
Dominik Kaszewski
Dominik Kaszewski

Reputation: 535

Reference counted singleton

I have have found the following code in my company's codebase. The logic behind is to free the config singleton when nobody is using it, as some users have been found to inflate its size, which has been measured to cause memory usage issues. While the obvious solution would be to force those users to clean up after themselves or not store data in singleton at all, ownership of the code made the following fix easier to implement as a workaround.

Aside from the code smell behind it, is it thread safe?

#include <memory>

class config {};

std::shared_ptr<config> get_config() {
    static std::weak_ptr<config> p;
    if (!p) {
        // No reference, create first instance
        return p = std::make_shared<config>();
    } else if (auto instance = p.lock()) {
        // Alive reference
        return instance;
    } else {
        // Expired reference, create new instance
        return p = std::make_shared<config>();
    }
}

I have read on the thread safety of shared_ptr and weak_ptr and found it a bit confusing. From what I understand they guarantee just the safety of the underlying block object and atomicity of the reference count, but it still not safe for multiple threads to modify the smart pointers themselves. This seems to be confirmed by the addition of std::atomic<std::weak_ptr<T>> in C++20, but how could I also solve this potential issue in C++17?

Upvotes: 3

Views: 90

Answers (3)

Alan Birtles
Alan Birtles

Reputation: 36488

No, it's not threadsafe, there's nothing to stop two threads either creating the first object or subsequently when the object needs to be recreated. std::weak_ptr and std::shared_ptr instances aren't threadsafe either so modifying them from multiple threads is undefined behaviour. You'll need to add a mutex to protect against this:

std::shared_ptr<config> get_config() {
    static std::weak_ptr<config> p;
    static std::mutex mutex;
    std::unique_lock lock(mutex);
    if (instance = p.lock()) {
        return instance;
    }
    return p = std::make_shared<config>();
}

Note that the initial if (!p) check is redundant, you can just call lock on an empty weak_ptr and it'll return null.

Upvotes: 2

Solomon Slow
Solomon Slow

Reputation: 27190

I would like someone to explain the different levels of thread safety behind the smart pointers and static variables, what is guaranteed by the standard, and what should be implemented by a mutex.

Forget "smart pointers and static variables" for a moment, and look at this:

    if (!p) {
        return p = ...
    }

Nothing prevents two threads from concurrently testing p, both finding it to effectively be false, and both assigning a new value to p. In your example, if *p effectively is immutable and, is constructed from persistent, immutable data, then the worst thing that happens is, your code constructs and destroys redundant copies of it. But, since your question starts with a concern about the "heavy" object, that could be a problem in and of itself.

Another answer here already talks about the thread safety of smart_ptr methods, so I won't go into that here.

P.S., This looks suspicious too, but I am not a C++ expert, so I'm not 100% sure:

if (auto instance = p.lock()) ...

My understanding of std::weak_ptr<T>::lock—based entirely on reading this documentation page, and not at all on my experience—is that there's no point in testing its return value because it always will return a valid pointer. If the weak_ptr was "expired" at the time of the call, then the call will default-construct a new instance and return a pointer to the new instance.

Again, since your code only ever calls the default constructor for config anyway, and if the config represents persistent, immutable data, then the worst that happens in your program is, it's constructing and destroying redundant copies of the configuration.

Upvotes: 3

463035818_is_not_an_ai
463035818_is_not_an_ai

Reputation: 123114

As you correctly write in your question the thread-safety of shared_ptr is restricted to its reference counting. Modifying the same shared_ptr (or weak_ptr) by two threads is not inherently thread safe and requires synchronisation. In different words from cppreference:

All member functions (including copy constructor and copy assignment) can be called by multiple threads on different shared_ptr objects without additional synchronization even if these objects are copies and share ownership of the same object. If multiple threads of execution access the same shared_ptr object without synchronization and any of those accesses uses a non-const member function of shared_ptr then a data race will occur; the std::atomic<shared_ptr> can be used to prevent the data race.

Note that the first part (thread-safety) is talking about different threads modifying different shared_ptr objects that share ownership of the same object. While the second part (no thread-safety) talks about two threads calling method on the same shared pointer.

I didn't find similar explicit notes about weak_ptr on cppreference, but the absence of a note saying it would be thread safe is enough to know it is not.

You need to synchronize modifying the same weak_ptr from different threads. Only the initialization is thread safe, because it is a function local static variable.

Upvotes: 2

Related Questions