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?
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.