Async race condition or bug?

Question

I'm trying to use atomic_bool to break out of an std::async, the structure of the code looks like below

class Test
{
   atomic_bool          IsStopped;
   std::future <void>   Future;

   Test ()
     : IsStopped (false);
   {
      LogValueWithMutex (IsStopped);

      Future = std::async (std::launch::async, &Test::AsyncFunction, this);
   }

   ~Test ()
   {
     LogValueWithMutex (IsStopped);

     IsStopped = true;

     if (Future.valid ())
       Future.get ();
   }

    void AsyncFunction ()
    {
      while (1)
      {
        // only read atomic_bool, does not write
        if (IsStopped)
          break;
      }
    }

    void CallbackFromNetworkWhichIsNotImportant ()
    {
      // Bug here!! On 2nd entry, this value is set to true! In Both Debug/Release Mode
      LogValueWithMutex (IsStopped);

      if (! IsStopped)
      {
        IsStopped = true;

        if (Future.valid ())
          Future.get ();

         // call an API which exit and will destroy this class
      }
    }
}

Everything works well which async can break properly. However, on 2nd creation of the class in VS2015, LogValueWithMutex in CallbackFromNetworkWhichIsNotImportant is true. I have verifed the log output it is

Test ctor IsStopped: 0
Test CallbackFromNetworkWhichIsNotImportant: 0
Test dtor IsStopped: 1
Test ctor IsStopped: 0
Test CallbackFromNetworkWhichIsNotImportant: 1 <- Wrong! Who is setting this to true??!!
Test dtor IsStopped: 1

IsStopped is only assigned to true in dtor and CallbackFromNetworkWhichIsNotImportant.

So I tried to check if it is an instance problem, I add a new random variable, and then it magically works!

class Test
{
   atomic_bool      IsStopped;
   std::future <void>   Future;
   std::string      RandomString; // HERE IS A TEST
   Test ()
     : IsStopped (false);
   {
        // NEW CODE
        ostringstream oss;
        oss << rand ()% 100;
        RandomStringToTestInstance = oss.str ();

        LogValueWithMutex (IsStopped);

        Future = std::async (std::launch::async, &Test::AsyncFunction, this);
   }
       // all parts unchanged
}

Test ctor IsStopped: 0
Test CallbackFromNetworkWhichIsNotImportant: 0
Test dtor IsStopped: 1
Test ctor IsStopped: 0
Test CallbackFromNetworkWhichIsNotImportant: 0 <- Correct!!
Test dtor IsStopped: 1

My understanding is atomic_bool does not need volatile, but somehow it looks like it is. Is there some kind of optimization or some auto generated move operator that can cause this?

Answer 1

I found the problem. It was nothing to do with async or atomic_bool (which I'm trying for the first time). I was assigning a callback based on "this" but was doing it once, so when I new the class again, it was using on existing instance (and hence corrupted memory). The multithreading with atomic_bool to break out of the loop works fine. Sorry for false alarm.

Answer 2

The usage of the code is missing. However I believe there is some multithreading going on there. Multithreading is tricky and can easily get into 'race conditions', i.e. cases in which the order of things is important and can become in some cases not as expected.

To investigate your story I created a simple usage that reproduces your unexpected output. It's not necessarily that you have the exact same scenario as I created below, there might be other similar race conditions. The code that reproduces your unexpected output is BAD (the object is being deleted in one thread while still being used in another thread) but bugs like that may happen in a multithreaded program. Look for such a bug in your code... (unless you are happy with the RandomString solution :-)

Here is the output that I get:

case 1:
Test::Test(): IsStopped = 0
void Test::CallbackFromNetworkWhichIsNotImportant(): IsStopped = 0
Test::~Test(): IsStopped = 1
case 2:
Test::Test(): IsStopped = 0
void Test::CallbackFromNetworkWhichIsNotImportant(): IsStopped = 1
Test::~Test(): IsStopped = 1

With the following main:

int main() {
    // OK scenario
    {
        cout << "case 1:" << endl;
        Test t;
        t.CallbackFromNetworkWhichIsNotImportant();
    }

    // Bad scenario
    std::this_thread::sleep_for(1s);
    cout << "case 2:" << endl;
    Test* tp = new Test();
    // here the error is quite verbose, but the same can be more hidden in the code
    std::thread th(&Test::CallbackFromNetworkWhichIsNotImportant, std::ref(*tp));
    delete tp;
    th.join();
}

Code: http://coliru.stacked-crooked.com/a/b8c0028766bcfc5c

Note Also

Note also that your CallbackFromNetworkWhichIsNotImportant function is said at some point to:

// call an API which exit and will destroy this class

it might be that the object is being destroyed twice, from two different places.