Singleton constructor with local static variable

Question

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;
}

I'm voting to close this question as off-topic because the question belongs to http://codereview.stackexchange.com — user3159253, Apr 13 '17 at 01:59
`static std::shared_ptr rest = std::make_shared(url); return rest;`. Your version is not thread_safe. — Jarod42, Apr 13 '17 at 08:07
@Jarod42, I understand that but I need to validate `url` prior creating `std::shared_ptr`. — Panch, Apr 13 '17 at 08:16
whereas `Singleton` is generally an anti-pattern, singleton with non default constructor is worse (as it depends of previous call)... — Jarod42, Apr 13 '17 at 08:26
@Jarod42, Yes, I understand that. The `std::shared_ptr ApiFacade::GetInstance(const std::string & url)` has default parameter. So you could just invoke `ApiFacade::GetInstance()`. — Panch, Apr 13 '17 at 08:47
I mean that your are not sure which url is used when you do `ApiFacade::GetInstance(myUrl);` (or `ApiFacade::GetInstance();`). — Jarod42, Apr 13 '17 at 08:51

Etherealone · Accepted Answer · 2017-04-13T17:05:21.457

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.

Edit:

I forgot to mention that, as @Jarod42 says, more than one ApiFacade class may get constructed. Only one will get to stay and the others will be destructed as a result of failing comparison to empty_rest.

Also, I should point out for the others that there is still some kind of logical race-y situation here. When the function is being called by multiple threads for the first time, if different url parameters are used by different threads, a random url will be successfully used while constructing the rest and others will be discarded. However, other than that, there is no undefined behavior so the code block is safe to use.

Not that several `ApiFacade` can be construct here. – Jarod42 Apr 13 '17 at 11:59 — Jarod42, Apr 13 '17 at 11:59

Singleton constructor with local static variable

1 Answers1