I saw this cpp core guideline and I'm trying to understand something:
Why is the first example marked as bad? Is it only because the variable is volatile? What harm can happen if the first check is not thread-safe assuming it's protected by the mutex? At the worst case we will stumble into a locked mutex but once it's unlocked we won't run the "only once" code.
Consider these 2 options:
class A{};
A() costly_init_function();
std::optional<A> a;
//option 1
std::once_flag a_init;
const A& foo() {
if (!a) {
std::call_once(a_init, [](){ a.emplace(costly_init_function()); });
}
return *a;
}
//option 2
std::atomic_bool a_init_flag = false;
std::mutex mutex;
const A& moo() {
if (!a_init_flag) {
std::lock_guard lock{mutex};
if (!a_init_flag) {
a.emplace(costly_init_function());
a_init_flag= true;
}
}
return *a;
}
Is there any actual issue that may happen in option 1? To me, it seems like the worst that could happen is that we access a in a not thread safe way and as a result we wait to the call_once to finish and then we simply skip to returning *a
.
Should I prefer option 2 which is more expensive but somewhat safer?
edit:
it seems people are thinking rephrasing my question and explaining it in more details is actually an answer. I would have just deleted the question but apparently I can't
so:
if (!a)
is not thread safestd::call_once
volatile
meanstrying to be more specific at what I'm asking:
bad
example it seemed to me that they are comparing apples to oranges because the used volatile.volatile
is essential to make the code "bad" or will using a regular storate variable also may cause an unwanted issueMaking the function thread-safe bears a cost. Yes, example 1 is faster, that is because it is not doing the required job.
There is a fundamental error in your thinking, that you can reason about concurrent accesses without atomics or syncing. The assumption is wrong. Any concurrent read and write from a non-atomic variable is UB.
The execution of a program contains a data race if it contains two potentially concurrent conflicting actions, at least one of which is not atomic, and neither happens before the other, except for the special case for signal handlers described below. Any such data race results in undefined behavior.
Note that "happens before" has a precise definition. Simply running function A 5 minutes after function B does not validate as "B happens before A".
In particular, a possible outcome there, is another thread sees a
as true before it sees the result of the init function.
The problem is not the volatile, it's the lack of sequencing. This cppcoreguideline only highlights the fact that volatile
does not offer sequencing guarantees, and as such exhibits the same undefined behavior as if the volatile
keyword was not there.
In short:
Eg:
int x = 0;
int y = 0;
void f() {
y = 1;
x = 1;
}
Assuming at least one thread runs this function and you have no other synchronisation mechanism in place, a thread reading those can observe y == 0
and x == 1
.
On a technical level:
f
has no reason to flush the writes to memory, and if it does it has no reason to do so in any particular order.x
and y
has no reason to invalidate its cache and fetch the modified values from memory, and if it does it has no reason to do so in any particular order (eg: it might have y
in cache but not x
, so it will load x
from memory and see the updated value, but use the cached y
).