C++ std::thread synchronization inside loop

Question

I made a mistake by choosing std::thread to make my thread concurrency assignment. I have no time left to learn some other method. So I hope you can help me out here, I'm really confused about all those types of mutexes, promises, futures etc. The lack of diverse documentation/tutorials is depressing. So I want 3 threads (players) to do the same thing - pick 2 random numbers as indexes for a global 2-D array, if the value of the cell is 0 set it equal to the thread's Id, otherwise stop execution of the thread. Do this in a loop until there's only one thread left. What I need is to find a way to synchronize the 3 threads after every (before the next) iteration so that one thread couldn't be on the 50th iteration, while the other is on, say, 30th, i.e., thread has to wait for all the others to complete the iteration before moving on. I need some kind of barrier. I tried using condition variables to put the threads to sleep until the last finishing thread signals to wake them up but I couldn't get it working. My unsuccessful tries are commented out:

std::mutex GridMutex, FinishMutex, LeftMutex;
int Grid[X][Y], Finished = 0, PlayersLeft = 3;

void Fight(int Id) {
  int RandX, RandY, i = 0;
  std::random_device rd; // access random device
  std::mt19937 e(rd()); // seed the engine
  std::uniform_int_distribution<int> r1(0, X-1);
  std::uniform_int_distribution<int> r2(0, Y-1);
  LeftMutex.lock(); // mutex on PlayersLeft
  while (PlayersLeft != 1) {
    ++i;
    /*std::call_once(flag, [Id, i](){*/std::cout << " Round " << i << "  Left: " << PlayersLeft << '\n';/*});*/
    LeftMutex.unlock();
//     FinishMutex.lock();
//     std::call_once(flag, [](){Finished = 0;});
//     FinishMutex.unlock();
    RandX = r1(e);
    RandY = r2(e);
    GridMutex.lock();
    if (Grid[RandX][RandY] != Id) {
      if (Grid[RandX][RandY] == 0) {
        Grid[RandX][RandY] = Id;
        std::cout << "i= "<< i << " Thread " << Id << " occupied cell " << RandX << RandY << '\n';
        std::chrono::milliseconds sleepDuration(100);
        std::this_thread::sleep_for(sleepDuration); //just to lock the grid for some time
        GridMutex.unlock();
      }
      else {
        GridMutex.unlock();
        LeftMutex.lock();
        --PlayersLeft;
        LeftMutex.unlock();
        break; //stop thread if cell occupied
      }
    }
    //Wait for the others here
//     FinishMutex.lock();
//     ++Finished;
//     if (Finished == 3) {
//       FinishMutex.unlock();
//       AllReady.notify_all();
//     }
//     else {
//       FinishMutex.unlock();
//       AllReady.wait(Lk, [](){return Finished == 3;});
//       Lk.unlock();
//     }
    LeftMutex.lock();
  }
}
int main() {
  SetGrid();
  std::thread t1(&Fight, 1);
  std::thread t2(&Fight, 2);
  std::thread t3(&Fight, 3);
  t1.join();
  t2.join();
  t3.join();
  GetGrid();
  return 0;
}

I expected the expressions inside the "call_once" thing to be executed by only one thread (whichever comes first) on every iteration but obiviously it's not what I thought it was because that results in some strange behaviour. How to do that? Ok, the code may be ugly and all that should be put inside classes and methods or something , but I just need to get this working no matter how primitively. Thank you in advance!

Answer 1

You could remove some locks by using atomic variables like std::atomic_int and std::atomic_bool (or std::atomic_flag.

Next to that, don't do the locking and unlocking yourself but use RAII wrappers for this, like std::lock_guard. This will lock the specified mutex until the lock object itself is destroyed (usually when it goes out of scope).

Now, while using those things will improve your code vastly, your actual logical error is quite simple: you should not compare to the constant 3 but to playersLeft. Note that as the condition is dependant on this you also have to signal whenever you change this variable.