1

Is this code thread safe?

static PFN_SOMEFUNC pfnSomeFunc = nullptr;

PFN_SOMEFUNC SomeFuncGetter();

void CalledFromManyThreads() {
    if (pfnSomeFunc == nullptr)
        pfnSomeFunc = SomeFuncGetter();

    pfnSomeFunc();
}

This is not atomic pointer operation question. There are some special conditions.

  • SomeFuncGetter() always returns same address.
  • SomeFuncGetter() is thread safe.
chaeyk
  • 491
  • 1
  • 8
  • 20
  • 1
    It's not. Wrap `pfunSomeFunc` with `std::atomic`. – Lingxi Oct 29 '21 at 02:36
  • 1
    You can make it a static *local* variable and benefit from [this awesome quirk](https://stackoverflow.com/a/8102145/8584929) in the C++ standard. – Andrej Podzimek Oct 29 '21 at 02:42
  • There is no synchronization used in the code, so there is no thread-safety in the code you posted. What happens outside this code -- this is the only code you posted and asked a question about. – PaulMcKenzie Oct 29 '21 at 03:19
  • @PaulMcKenzie pfnSomeFunc is modified by CalledFromManyThreads() function. And pfnSomeFunc can have only one value. Can there be a synchronization problem? – chaeyk Oct 29 '21 at 03:30
  • 1
    It is not thread-safe. `pfnSomeFunc = SomeFuncGetter();` The `=` is not thread-safe in itself. Maybe, just maybe 30 years ago, the code was "thread-safe" in the sense that processors were less sophisticated. But in this day and age, comparison and assignment (plus using mere `boolean` flags) is not going to get you thread-safety. Only the use of proper synchronization is going to give you those results. – PaulMcKenzie Oct 29 '21 at 03:45
  • 2
    @chaeyk Yes, there is a "synchronization problem". Your assertion that `pfSomeFunc` can only have one value is incorrect - the code specifies two values (`nullptr` on initialisation, the value returned by `SomeFuncGetter()` on a subsequent assignment). Since there is no synchronisation in this code, race conditions exist affecting both the comparison of `pfSomeFunc` with `nullptr` and the assignment - either (or both) of those operations may be pre-empted (since neither operation is atomic) by another thread when partially complete. – Peter Oct 29 '21 at 03:45
  • @Peter -- A couple years ago, I worked for a company where the head programmer from the 1980's couldn't wrap their head around the fact that you couldn't achieve thread-safety by doing a comparison and setting a couple of `bool` variables. Frustrating to say the least. – PaulMcKenzie Oct 29 '21 at 03:55
  • @Peter Good! Thank you. – chaeyk Oct 29 '21 at 04:16

1 Answers1

3

It doesn't look thread-safe, because the global variable can be modified by any thread without synchronization. Even an assignment is not guaranteed to be atomic.

What you could do is leverage a language feature that guarantees thread-safe atomic initialization by moving the static into your function (a very common solution to the Static Initialization Fiasco):

void CalledFromManyThreads() {
    static PFN_SOMEFUNC pfnSomeFunc = SomeFuncGetter();
    pfnSomeFunc();
}

Now it's thread-safe. And if you need that cached result in multiple places, you can wrap it:

PFN_SOMEFUNC GetSomeFunc()
{
    static PFN_SOMEFUNC pfnSomeFunc = SomeFuncGetter();
    return pfnSomeFunc;
}

void CalledFromManyThreads() {
    PFN_SOMEFUNC pfnSomeFunc = GetSomeFunc();
    pfnSomeFunc();
}
paddy
  • 60,864
  • 6
  • 61
  • 103
  • 1
    It's true it's thread safe as per the C++11 standard. It wasn't thread-safe for earlier than C++11. – PaulMcKenzie Oct 29 '21 at 03:15
  • With visual studio 2010, some threads get null value from static pfnSomeFunc. – chaeyk Oct 29 '21 at 04:12
  • 1
    VS 2010 does not support C++11 – paddy Oct 29 '21 at 04:44
  • 1
    @chaeyk -- You need to be on Visual Studio 2015, SP 3 or later to see the full effects of what paddy is showing you. Earlier versions of Visual Studio lacked this feature. [See this question and answer](https://stackoverflow.com/questions/28096970/when-is-a-divide-by-zero-not-a-divide-by-zero-a-puzzle-in-the-debugger-static) concerning "magic statics" and the support for them in Visual Studio. – PaulMcKenzie Oct 29 '21 at 13:46
  • @chaeyk -- Also, I would recommend not using VS 2010 for development if you really want to use C++11. The compiler has several C++11 "features" that are actually broken (like brace initialization rules and lambdas) -- so if you use any C++11 idioms using that compiler, you're risking things. – PaulMcKenzie Oct 29 '21 at 13:56