CODEWITHSUNDEEP
c++multithreadingc++11thread-safety
Krazer
Krazer

Reputation: 515

Mutex and variable updates

Need verification of a thought process, say I set up a thread as follows

bool threadRun=true;
std::mutex threadMutex;
std::condition_variable event;
std::thread processThread(process);

void process()
{
    while(threadRun)
    {
        if(noWork())
        {
            std::unique_lock<std::mutex> lock(threadMutex);

            if(threadRun)
            {
                if(!getWork()) //try to get work, return false if no work
                    event.wait(lock);
            }

            continue;
        }

        doWork();
    }
}

void stopThread()
{
    {
        std::unique_lock<std::mutex> lock(threadMutex);
        threadRun=false;
    }
    event.notify_all();
    processThread.join();
}

Since threadRun is not under a mutex lock in the thread there is no guarantee that calling stopThread will actually stop the thread as no cache flush is required when there is work todo.However when there is no work and the lock is taken this should cause a cache flush and threadRun is guaranteed to be updated, correct?

Upvotes: 1

Views: 67

Answers (1)

Yakk - Adam Nevraumont
Yakk - Adam Nevraumont

Reputation: 275385

Data races in C++ are undefined behavior.

Now, you might think this means "well, I'm ok with them missing a read, they'll get it eventually". But UB is something that the compiler is free to assume will not happen.

The fact you are reading threadRun without a lock or other synchronization means that the compiler can, may and should assume that nobody in any other thread will modify threadRun.

Thus the code can be optimized to:

if(threadRun)
  while(true)

ie, the compiler can read threadRun once, and then don't bother reading it again if it can prove that nobody could change its value in a defined way.

Now, if the compiler cannot prove this, it instead has to read threadRun. So your UB acts more like it "should" work. And on some platforms, the hardware itself has cached threadRun in a per-processor (or thread) cache, and the fact that another thread wrote to it doesn't get reflected in your worker thread for some unknown period of time (possibly forever).

Going further, a smart compiler could note that you are reading threadRun without synchronization. It could then use that to prove that nobody, with or without synch, could write to threadRun in another thread.

Once it has proven that, it can elimiate the check of threadRun within the mutex as well.

Don't do undefined behavior unless you want to validate the assembly your program produces from now until the end of time every time you compile it, even if it "works", or the gain is huge and worth the risk.

Replace threadRun with std::atomic_bool. You'll lose a tiny bit of performance in the worst case and gain correctness.

Upvotes: 1

Related Questions