0

I want to create 15 threads and have them performed 4 successive steps (that I call Init, Process, Terminate and WriteOutputs).
For each step I want all threads to finish it before passing to the following step.

I am trying to implement it (cf code below) using a std::condition_variable and calling the wait() and notify_all() methods but somehow I do not manage to do it and even worse I have a race condition when counting the number of operations done (which should be 15*4 = 60) I sometimes have some prints that are indeed not printed and the m_counter in my class at the end is less than 60 which should not be the case

I use two std::mutex objects: one for printing messages and another one for the step synchronization

Could someone explain to me the problem?
What would be a solution ?
Many thanks in advance

#include<iostream>
#include<thread>
#include<mutex>
#include<condition_variable> 
#include<vector>
#include<functional>

class MTHandler
{ 
  public:
    MTHandler(){
      // 15 threads 
      std::function<void(int)> funcThread = std::bind(&MTHandler::ThreadFunction, this, std::placeholders::_1);
      for (int i=0; i<15; i++){
        m_vectThreads.push_back(std::thread(funcThread,i));
      }
      for (std::thread & th : m_vectThreads) {
        th.join();
      }

      std::cout << "m_counter = " << m_counter << std::endl; 
    }
  
  private: 
     enum class ManagerStep{
      Init, 
      Process,
      Terminate,
      WriteOutputs,
    };

    std::vector<ManagerStep> m_vectSteps = {
      ManagerStep::Init, 
      ManagerStep::Process,
      ManagerStep::Terminate,
      ManagerStep::WriteOutputs
    };

    unsigned int m_iCurrentStep = 0 ; 

    unsigned int m_counter = 0;

    std::mutex m_mutex;
    std::mutex m_mutexStep;
    std::condition_variable m_condVar;

    bool m_finishedAllSteps    = false;
    unsigned int m_nThreadsFinishedStep = 0;
    std::vector<std::thread> m_vectThreads = {};

    void ThreadFunction (int id) {
      while(!m_finishedAllSteps){
        m_mutex.lock();
        m_counter+=1;
        m_mutex.unlock();
        switch (m_vectSteps[m_iCurrentStep])
        {
          case ManagerStep::Init:{
            m_mutex.lock();
            std::cout << "thread " << id  << " --> Init step" << "\n";
            m_mutex.unlock();
            break;
          }
          case ManagerStep::Process:{
            m_mutex.lock();
            std::cout << "thread " << id  << " --> Process step" << "\n";
            m_mutex.unlock();
            break;
          }
          case ManagerStep::Terminate:{
            m_mutex.lock();
            std::cout << "thread " << id  << " --> Terminate step" << "\n";
            m_mutex.unlock();
            break;
          }
          case ManagerStep::WriteOutputs:{
            m_mutex.lock();
            std::cout << "thread " << id  << " --> WriteOutputs step" << "\n";
            m_mutex.unlock();
            break;
          }
          default:
          {
            break;
          }
        }

        unsigned int iCurrentStep = m_iCurrentStep; 
        bool isCurrentStepFinished = getIsFinishedStatus();
        if (!isCurrentStepFinished){
          // wait for other threads to finish current step
          std::unique_lock<std::mutex> lck(m_mutexStep);
          m_condVar.wait(lck, [iCurrentStep,this]{return iCurrentStep != m_iCurrentStep;});
        }
      }
    }

    bool getIsFinishedStatus(){
      m_mutexStep.lock();
      bool isCurrentStepFinished = false;
      m_nThreadsFinishedStep +=1;
      if (m_nThreadsFinishedStep == m_vectThreads.size()){
        // all threads have completed the current step 
        // pass to the next step
        m_iCurrentStep += 1;
        m_nThreadsFinishedStep = 0;
        m_finishedAllSteps = (m_iCurrentStep == m_vectSteps.size());
        isCurrentStepFinished = true;
      }
      if (isCurrentStepFinished){m_condVar.notify_all();}
      m_mutexStep.unlock();
      return isCurrentStepFinished;
    }
};

int main ()
{
  
  MTHandler mt;

  return 0;
}
romain.bqt4
  • 159
  • 1
  • 6
  • 1
    If you can use C++20 features, see [std::barrier](https://en.cppreference.com/w/cpp/thread/barrier). It does exactly what you want. – Igor Tandetnik Feb 13 '22 at 15:27
  • @IgorTandetnik thanks but I am using C++11 But do you know what is the problem ? Because I don't understand what is wrong in my code – romain.bqt4 Feb 13 '22 at 15:28
  • 1
    Right off the top, you have a race on `m_finishedAllSteps`. It's updated under the mutex, but is read without, so it's possible for one thread to update it while the other checks it. Same with `m_iCurrentStep`. – Igor Tandetnik Feb 13 '22 at 15:33
  • 1
    Also a race on `m_vectThreads`. It's possible that the first thread gets all the way to `getIsFinishedStatus` and calls `m_vectThreads.size()` while the main thread is still pushing more elements onto `m_vectThreads` vector. – Igor Tandetnik Feb 13 '22 at 15:40
  • 1
    Upon closer inspection, `m_finishedAllSteps` and `m_iCurrentStep` are not a problem after all; they should only be updated when all but one thread are blocking on the condition variable (that one thread is the last one to complete the step, and is the one doing the updating). Most likely, the problem is the race on `m_vectThreads` - it causes the first step to complete prematurely, and from there the whole carefully coordinated sequence falls apart. Does the program work if you hard-code `15` in place of `m_vectSteps.size()`? – Igor Tandetnik Feb 13 '22 at 15:50
  • @IgorTandetnik thanks a lot !!! Yes I think you are right the real problem should be coming from the `m_vectThreads` – romain.bqt4 Feb 13 '22 at 15:54
  • Yes it does work ! Thanks again @IgorTandetnik – romain.bqt4 Feb 13 '22 at 16:00

0 Answers0