One thread sets member while the other loops over it – Is this thread-unsafe?

  • A+

I just discovered the following construct in our codebase (simplified in the example):

class SomeClass {   public:     void setKeepGoing(bool b) { m_keepGoing = b; }     void setDoAdditionalStuff(bool b) { m_doAdditionalStuff = b; }     void someLoop()     {         while(m_keepGoing)          {             //Do something             bool doMore = m_doAdditionalStuff;             if (doMore)                 //Do more things         }       }    private:     bool m_keepGoing;     bool m_doAdditionalStuff; } 

There are multiple threads, one calling someLoop() while the others call setKeepGoing() and/or setDoAdditionalStuff().

Now my sinking gut feeling is that this is horribly thread-unsafe. The compiler may very well optimize away reading m_doAdditionalStuff inside the loop (as it is not changed there) and even m_keepGoing (as that too is not changed there) effectively resulting in code acting like:

void someLoop() {     if (!m_keepGoing)         return;     bool doMore = m_doAdditionalStuff;     while(true)      {         //Do something         if (doMore)             //Do more things     }   } 

Am I correct in my suspicions?


Your suspicions are correct. You cannot write and read from the same variable in multiple threads without some sort of synchronization mechanism. Doing so is a data race and is undefined behavior.

What you can do in this case is to use a std::atomic<bool> for m_keepGoing and m_doAdditionalStuff so that you will have synchronization.


:?: :razz: :sad: :evil: :!: :smile: :oops: :grin: :eek: :shock: :???: :cool: :lol: :mad: :twisted: :roll: :wink: :idea: :arrow: :neutral: :cry: :mrgreen: