First and foremost, I understand that formally, using a non-atomic flag to cancel a thread is very much undefined behaviour in the sense that the language does not specify if this variable will be written to before the thread exits.
At work, this was implemented a long time ago, and most calculation threads check the value of this bool throughout their work, as to gracefully cancel whatever it is they're doing. When I first saw this, my first reaction was to change all of this to use a better way (in this case, QThread::requestInterruption and QThread::interruptionRequested seemed like a viable alternative). A quick search through the code turned up about 800 occurences of this variable/construct throughout the codebase, so I let it go.
When I approached a (senior, in terms of years of experience) colleague, he assured me that although it might indeed be wrong, he had never seen it fail to fulfill its purpose. He argued that the only case it would go wrong is if a (group of) thread(s) is allowed to run and another thread that actually changes this flag never gets allowed to execute untill the other threads are finished. He also argued that in this case, the OS would intervene and fairly distribute runtime across all threads, resulting in perhaps a delay of the cancellation.
Now my question is: is there any real-life situation (preferably on a regular system, based upon x86/ARM, preferably C or C++) where this does indeed fail?
Note I'm not trying to win the argument, as my colleague agrees it is technically incorrect, but I would like to know if it could cause problems and under which circumstances this might occur.
The simplest way to beat this is to reduce it to a rather trivial example. The compiler will optimize out reading the flag because it is not atomic and being written to by another thread is UB; therefore the flag won't ever get actually read.
Your colleague's argument is predicated on the assumption that the compiler will actually load the flag when you de-reference the flag. But in fact it has no obligation to do so.
#include <thread>
#include <iostream>
bool cancelled = false;
bool finished = false;
void thread1() {
while(!cancelled) {
std::cout << "Not cancelled";
}
}
int main() {
std::thread t(thread1);
t.detach();
cancelled = true;
while(!finished) {}
}
To run on coliru, load http://coliru.stacked-crooked.com/a/5be139ee34bf0a80, you will need to edit and make a trivial change because the caching is broken for snippets that do not terminate.
Effectively, he's simply betting that the compiler's optimizer will do a poor job, which seems like a truly terrible thing to rely upon.