0

I recently came across some code that was working fine where a static bool was shared between multiple threads (single writer, multiple receivers) although there was no synchronization.

Something like that (simplified):

//header A
struct A {
   static bool f;
   static bool isF() { return f; }
};

//Source A
bool A::f = false;

void threadWriter(){
    /* Do something */
    A::f = true;
}

// Source B
void threadReader(){
   while (!A::isF()) { /* Do something */}
}

For me, this kind of code has a race condition in that even though operations on bool are atomic (on most CPUs), we have no guarantee that the write from the writer thread will be visible to the reader threads. But some people told me that the fact that f is static would help.

So, is there anything in C++11 that would make this code safe? Or anything related to static that would make this code work?

Baptiste Wicht
  • 7,472
  • 7
  • 45
  • 110
  • 1
    No, this code causes UB. – HolyBlackCat Jul 09 '21 at 17:09
  • 2
    std::atomic? Also, even if your bool was atomic, calling isF() to read it probably isn't. – AlexG Jul 09 '21 at 17:09
  • 1
    This is an old-school attitude and may not be accurate, but I've lived with the belief that if it needs to be thread-safe, it needs a mutex. I'll be watching to see if someone posts a better answer. – Joseph Larson Jul 09 '21 at 17:13
  • The C++ standard does not guarantee that operations on `bool` are atomic. If you want atomic operations use `std::atomic`. The implementation will manage the complexities of atomicity, which go beyond the first-level issue of whether a store operation can be torn. – Pete Becker Jul 09 '21 at 17:15
  • 2
    @JosephLarson -- `std::atomic` provides thread-safe operations on a single object. If that's sufficient, there's no need for a mutex, and the atomic operations may well be faster. – Pete Becker Jul 09 '21 at 17:16
  • @PeteBecker I'm not sure what AlexG's comment about the isF() function implies then, or maybe he's mistaken. – Joseph Larson Jul 09 '21 at 17:18
  • And it can go a step beyond atomicity of access. Without extra guards like `std::atomic` the compiler is often within it's rights to say "No one changes the value in this loop, so I'll just optimize this test out and loop forever." – user4581301 Jul 09 '21 at 17:18
  • An `atomic` is not sufficient for this application, at least not the way it is used. You would need to compare-exchange atomically, otherwise it does not protect against simultaneous writes. The flag could be `false` in the condition for multiple threads before any of them set it to `true`. The flag must atomically be set to `true` just as it is measured to be `false` or you still have a race condition. – François Andrieux Jul 09 '21 at 17:22
  • @JosephLarson -- it's not clear from the code what the actual use is intended to be. It's not so much the function `ifF()` as how it's used. – Pete Becker Jul 09 '21 at 17:25
  • There are two separate issues here. The title asks whether there's a way to make `bool` thread safe, and the answer is `std::atomic`. There's an additional implied question of how to make the code in the question thread safe, and that's more complicated, in part because the code has been greatly simplified in order to ask the title question. – Pete Becker Jul 09 '21 at 17:36
  • 1
    actually I dont quite understand the question. Is it "This code smells, but it works. Why?" or is it "This code smells, how to fix it?" – 463035818_is_not_an_ai Jul 09 '21 at 17:38
  • 1
    I think now I get it. Question is "Is the bool thread-safe in this code because it is `static`?", right? Answer is No – 463035818_is_not_an_ai Jul 09 '21 at 17:42
  • The comments on [A: Lock free synchronization](https://stackoverflow.com/a/9104017) appear relevant. – JaMiT Jul 09 '21 at 17:58
  • Yes, the question was whether static had anything to do on why this code is working. I know how to fix it (std:.atomic). Thanks all! – Baptiste Wicht Jul 10 '21 at 06:43

1 Answers1

3

Your hardware may be able to atomically operate on a bool. However, that does not make this code safe. As far as C++ is concerned, you are writing and reading the bool in different threads without synchronisation, which is undefined.

Making the bool static does not change that.

To access the bool in a thread-safe way you can use a std::atomic<bool>. Whether the atomic uses a mutex or other locking is up to the implementation.

Though, also a std::atomic<bool> is not sufficient to synchronize the threadReader() and threadWriter() in case each /*Do something */ is accessing the same shared data.


But some people told me that the fact that f is static would help.

Frankly, this sounds like cargo-cult. I can imagine that this was confused with the fact that initialization of static local variables is thread safe. From cppreference:

If multiple threads attempt to initialize the same static local variable concurrently, the initialization occurs exactly once (similar behavior can be obtained for arbitrary functions with std::call_once).

Note: usual implementations of this feature use variants of the double-checked locking pattern, which reduces runtime overhead for already-initialized local statics to a single non-atomic boolean comparison.

Look for Meyers singleton to see an example of that. Though, this is merely about initialization. For example here:

 int& foo() {
     static int x = 42;
     return x;
 }

Two threads can call this function concurrently and x will be initialized exactly once. This has no impact on thread-safety of x itself. If two threads call foo and one writes and another reads x there is a data race. However, this is about initialization of static local variables and has nothing to do with your example. I don't know what they meant when they told you static would "help".

463035818_is_not_an_ai
  • 109,796
  • 11
  • 89
  • 185
  • 1
    This is all correct. But even with `std::atomic` the code contains other errors. Using an `std::atomic` does not adequately protect against race conditions between `threadWriter` and `threadReader`. For one, `threadWriter` needs to somehow wait that there is no active `threadReader`. – François Andrieux Jul 09 '21 at 17:28
  • @FrançoisAndrieux isnt using `store` and `load` enough synchronization? Sorry, I am not sure if I understand the code completely (and your comment) – 463035818_is_not_an_ai Jul 09 '21 at 17:29
  • The key here is that "atomically" in "hardware may be able to atomically operate on a `bool`" is not the same as "atomically" in `std::atomic` supports operating atomically on a `bool` object. `std::atomic` manages potential data races from the perspective of the C++ language, and, properly used, ensures that there are no data races. – Pete Becker Jul 09 '21 at 17:30
  • @PeteBecker thats bascially what I wanted to say, just better phrased ;) – 463035818_is_not_an_ai Jul 09 '21 at 17:30
  • @463035818_is_not_a_number It is enough to avoid the UB from accessing the flag. But the scheme used in the question does not synchronize the data manipulated by `threadWriter` and `threadReader` (assuming they touch the same data). If a thread calls `threadWriter` while another is running `threadReader` then there is nothing preventing a race on that shared data. – François Andrieux Jul 09 '21 at 17:32
  • @FrançoisAndrieux if you refer to shared data not shown in OPs code, then I understand what you mean. – 463035818_is_not_an_ai Jul 09 '21 at 17:34
  • @463035818_is_not_a_number Yes, I'm referring to the hypothetical data these member functions would manipulate, but which was not shown in the question. – François Andrieux Jul 09 '21 at 17:34
  • To clarify @FrançoisAndrieux you're approaching from a safe transaction point of view rather than merely safe checking of the bool after which `/* Do Something */'` must be safe to execute unsynchronized, yes? – user4581301 Jul 09 '21 at 19:00
  • Thanks for the detailed answer @463035818_is_not_a_number! Regarding the rest of the code, let's assume that it's all local. And I am not looking into a solution that would make it thread-safe. Just looking at whether this kind of code is supposed to be working (it's not!). – Baptiste Wicht Jul 10 '21 at 06:45