Why my std::atomic<int> variable isn't thread-safe?

Question

I don't know why my code isn't thread-safe, as it outputs some inconsistent results.

value 48
value 49
value 50
value 54
value 51
value 52
value 53

My understanding of an atomic object is it prevents its intermediate state from exposing, so it should solve the problem when one thread is reading it and the other thread is writing it.

I used to think I could use std::atomic without a mutex to solve the multi-threading counter increment problem, and it didn't look like the case.

I probably misunderstood what an atomic object is, Can someone explain?

void
inc(std::atomic<int>& a)
{
  while (true) {
    a = a + 1;
    printf("value %d\n", a.load());
    std::this_thread::sleep_for(std::chrono::milliseconds(1000));
  }
}

int
main()
{
  std::atomic<int> a(0);
  std::thread t1(inc, std::ref(a));
  std::thread t2(inc, std::ref(a));
  std::thread t3(inc, std::ref(a));
  std::thread t4(inc, std::ref(a));
  std::thread t5(inc, std::ref(a));
  std::thread t6(inc, std::ref(a));

  t1.join();
  t2.join();
  t3.join();
  t4.join();
  t5.join();
  t6.join();
  return 0;
}

Answer 1

I used to think I could use std::atomic without a mutex to solve the multi-threading counter increment problem, and it didn't look like the case.

You can, just not the way you have coded it. You have to think about where the atomic accesses occur. Consider this line of code …

a = a + 1;

1. First the value of a is fetched atomically. Let's say the value fetched is 50.
2. We add one to that value getting 51.
3. Finally we atomically store that value into a using the = operator
4. a ends up being 51
5. We atomically load the value of a by calling a.load()
6. We print the value we just loaded by calling printf()

So far so good. But between steps 1 and 3 some other threads may have changed the value of a - for example to the value 54. So, when step 3 stores 51 into a it overwrites the value 54 giving you the output you see.

As @Sopel and @Shawn suggest in the comments, you can atomically increment the value in a using one of the appropriate functions (like fetch_add) or operator overloads (like operator ++ or operator +=. See the std::atomic documentation for details

Update

I added steps 5 and 6 above. Those steps can also lead to results that may not look correct.

Between the store at step 3. and the call tp a.load() at step 5. other threads can modify the contents of a. After our thread stores 51 in a at step 3 it may find that a.load() returns some different number at step 5. Thus the thread that set a to the value 51 may not pass the value 51 to printf().

Another source of problems is that nothing coordinates the execution of steps 5. and 6. between two threads. So, for example, imagine two threads X and Y running on a single processor. One possible execution order might be this …

1. Thread X executes steps 1 through 5 above incrementing a from 50 to 51 and getting the value 51 back from a.load()
2. Thread Y executes steps 1 through 5 above incrementing a from 51 to 52 and getting the value 52 back from a.load()
3. Thread Y executes printf() sending 52 to the console
4. Thread X executes printf() sending 51 to the console

We've now printed 52 on the console, followed by 51.

Finally, there's another problem lurking at step 6. because printf() doesn't make any promises about what happens if two threads call printf() at the same time (at least I don't think it does).

On a multiprocessor system threads X and Y above might call printf() at exactly the same moment (or within a few ticks of exactly the same moment) on two different processors. We can't make any prediction about which printf() output will appear first on the console.

Note The documentation for printf mentions a lock introduced in C++17 "… used to prevent data races when multiple threads read, write, position, or query the position of a stream." In the case of two threads simultaneously contending for that lock we still can't tell which one will win.

Answer 2

Besides the increment of a being done non-atomically, the fetch of the value to display after the increment is non-atomic with respect to the increment. It is possible that one of the other threads increments a after the current thread has incremented it but before the fetch of the value to display. This would possibly result in the same value being shown twice, with the previous value skipped.

Another issue here is that the threads do not necessarily run in the order they have been created. Thread 7 could execute its output before threads 4, 5, and 6, but after all four threads have incremented a. Since the thread that did the last increment displays its output earlier, you end up with the output not being sequential. This is more likely to happen on a system with fewer than six hardware threads available to run on.

Adding a small sleep between the various thread creates (e.g., sleep_for(10)) would make this less likely to occur, but would still not eliminate the possibility. The only sure way to keep the output ordered is to use some sort of exclusion (like a mutex) to ensure only one thread has access to the increment and output code, and treat both the increment and output code as a single transaction that must run together before another thread tries to do an increment.