Is the implementation of bidirectional single point thread safe?
I know the general implementation of a thread-safe singleton looks like this:
Singleton* Singleton::instance() {
if (pInstance == 0) {
Lock lock;
if (pInstance == 0) {
Singleton* temp = new Singleton; // initialize to temp
pInstance = temp; // assign temp to pInstance
}
}
return pInstance;
}
But why do they say that this is a thread-safe implementation?
For example, the first thread can pass both tests for pInstance == 0
, create new Singleton
and assign it to a pointer temp
, and then start the assignment pInstance = temp
(to my knowledge, the operation of assigning a pointer is not atomic).
At the same time the second thread checks the first pInstance == 0
where pInstance
only half is assigned. It is not nullptr, but not a valid pointer, which is then returned from the function. Could this situation happen? I haven't found an answer anywhere and it seems to be a perfectly correct implementation and I don't understand anything
source to share
It is not safe with C ++ concurrency rules, as the first read is pInstance
not protected by a lock or something like that and therefore does not sync correctly with the record (which is protected). Hence there is a data race and hence Undefined Behavior. One of the possible outcomes of this UB is exactly what you defined: the first check that reads a garbage value pInstance
that is just written by another thread.
The general explanation is that it saves on acquiring a lock (a potentially costly operation) more generally ( pInstance
already valid). However, this is not safe.
Since C ++ 11 and up guarantee function-scope initialization, static variables only happen once and are thread safe, the best way to create a singleton in C ++ is to have a static local in the function:
Singleton& Singleton::instance() {
static Singleton s;
return s;
}
Note that there is no need for dynamic allocation or pointer return type.
Like Voo mentioned in the comments, the above assumes it pInstance
is a raw pointer. If it were std::atomic<Singleton*>
, then the code would work exactly as intended. Of course the question then is whether atomic reading is much slower than acquiring a lock, which should be answered via profiling. However, this would be a rather pointless exercise, since static local variables are better in all but very obscure cases.
source to share