Does this use of release/acquire semantics contain a potential data race?

Question

Consider the following code which I found on http://preshing.com/20120913/acquire-and-release-semantics/ (but I am sure I saw it frequently elsewhere)

Common code:

int A = 0;
std::atomic<int> Ready(0);

Code executed by Thread 1:

A = 42
Ready.store(1, std::memory_order_release);

Code executed by Thread 2:

int r1 = Ready.load(std::memory_order_acquire);
int r2 = A;

It can then be said, that if r1 == 1 then r2 will always be 42 and there is no data race.

My first question is: Does the code nevertheless contain a data race? I mean in the case that r1 == 0. For example, Thread 1 could be preempted half-way through the store to A and then Thread 2 could pick up execution.

I would think that the code for Thread 2 should be rewritten as:

int r1 = Ready.load(std::memory_order_acquire);
int r2 = 0;
if( r1 == 1 ) {
    r2 = A;
}

to avoid the potential data race.

Is that true?

Answer 1

Yes, your interpretation is correct: the program contains a data race due to the unconditional read of A. The program is simplified to be a minimal example to demonstrate the workings of acquire-release for the blog: the author discusses only how the memory ordering enforces that the "reader" thread must read 42 from A if it reads 1 from Ready. He doesn't talk about the alternative because it's not germaine.

In a real program, the "reader" thread would probably wait in a loop on Ready to get the desired semantics:

int A = 0;
std::atomic<int> Ready(0);

void write_thread() {
  A = 42;
  Ready.store(1, std::memory_order_release);
}

void read_thread() {
  while (!Ready.load(std::memory_order_acquire))
    ;
  int r2 = A;
  assert(r2 == 42);
}

Or use a conditional as in your example:

int A = 0;
std::atomic<int> Ready(0);

void write_thread() {
  A = 42;
  Ready.store(1, std::memory_order_release);
}

void read_thread() {
  while (true) {
    if (Ready.load(std::memory_order_acquire)) {
      int r2 = A;
      assert(r2 == 42);
      break;
    }

    std::cout << "Not ready yet - try again later.\n" << std:flush;
    std::this_thread::sleep_for(std::chrono::milliseconds{125});
  }
}