Panch Panch - 7 months ago 28
C++ Question

Singleton constructor with local static variable

I'm doing an exercise with Singleton design pattern. From the discussions here and here, I understand the initialization of local static variable is thread safe since C++11. Please consider the code snippet below,

std::shared_ptr<ApiFacade> ApiFacade::GetInstance(const std::string & url)
{
// Initialization of function-local statics is guaranteed to occur only
// once even when called from multiple threads, and may be more efficient
// than the equivalent code using std::call_once.
static std::shared_ptr<ApiFacade> rest = nullptr;
if (rest == nullptr)
{
if (!url.empty())
{
rest = std::shared_ptr<ApiFacade>(new ApiFacade(url));
}
else
{
throw std::invalid_argument("Failed creating REST API object, Pass a valid URL");
}
}

return rest;
}


Here the local static is initialized with
nullptr
first and then assigned with the appropriate pointer using
rest = std::shared_ptr<ApiFacade>(new ApiFacade(url));
. I want to know whether the thread safe is ensured until the local
static
is assigned with an instance of
ApiFacade
or should I still need to use the DCLP for this case. Here the local
static
initialization is done with
nullptr
and later with an instance of
ApiFacade
.

However I tried to address the race condition as below but @Etherealone solution looks good

{
static std::shared_ptr<ApiFacade> rest = nullptr;

if (rest == nullptr)
{
// Mutex to provide exclusive access to resource.
std::recursive_mutex singleTonMtx = {};
std::lock_guard<std::recursive_mutex> lock(singleTonMtx);
if (!url.empty() && (rest == nullptr))
{
rest = std::shared_ptr<ApiFacade>(new ApiFacade(url));
}
else
{
throw std::invalid_argument("Failed creating API object, Pass a valid URL");
}
}

return rest;
}

Answer Source

No it is not thread-safe. Thread-safety ends with the initialization of variable rest with nullptr. Multiple threads can run the code following that in parallel.

The thread-safe version would be this:

std::shared_ptr<ApiFacade> ApiFacade::GetInstance(const std::string & url) {
    // Initialization of function-local statics is guaranteed to occur only
    // once even when called from multiple threads, and may be more efficient
    // than the equivalent code using std::call_once.
    static std::shared_ptr<ApiFacade> rest = nullptr;

    if (rest == nullptr)
    {
        if (!url.empty())
        {
            std::shared_ptr<ApiFacade> empty_rest;
            std::atomic_compare_exchange_strong(&rest, &empty_rest, std::make_shared<ApiFacade>(url));
        }
        else
        {
            throw std::invalid_argument("Failed creating REST API object, Pass a valid URL");
        }
    }

    return rest;
}

But this only updates the rest if it is empty. I am not sure if that is the desired behaviour.