Multiple Conditional Variables : threads out of sync problem

Question

I have a thread that is doing "work", it is supposed to report progress when conditional variable notifies it. This thread is waiting for conditional variables.

Other thread is waiting for a x amount of milliseconds and then notifies conditional variable to proceed.

I have 5 conditional variables (this is an exercise for school) and once each gets notified work progress is supposed to be reported:

Problem im having is that thread 2, the one that is supposed to notify thread 1, goes through all 5 checkPoints and notifies only once in the end. So I end up in a situation where progress is at 20% in the end and thread 1 is waiting for another notify but thread 2 has finished all notifies.

Where is flaw in my implementation of this logic? Code below:

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

using namespace std;

class Program {
public:
  Program() {
    m_progress = 0;
    m_check = false;
  }

  bool isWorkReady() { return m_check; }

  void loopWork() {
    cout << "Working ... : " << endl;

    work(m_cv1);
    work(m_cv2);
    work(m_cv3);
    work(m_cv4);
    work(m_cv5);

    cout << "\nFinished!" << endl;
  }

  void work(condition_variable &cv) {
    unique_lock<mutex> mlock(m_mutex);
    cv.wait(mlock, bind(&Program::isWorkReady, this));
    m_progress++;
    cout << " ... " << m_progress * 20 << "%" << endl;
    m_check = false;
  }

  void checkPoint(condition_variable &cv) {
    lock_guard<mutex> guard(m_mutex);
    cout << " < Checking >" << m_progress << endl;
    this_thread::sleep_for(chrono::milliseconds(300));
    m_check = true;
    cv.notify_one();
  }

  void loopCheckPoints() {
    checkPoint(m_cv1);
    checkPoint(m_cv2);
    checkPoint(m_cv3);
    checkPoint(m_cv4);
    checkPoint(m_cv5);
  }

private:
  mutex m_mutex;
  condition_variable m_cv1, m_cv2, m_cv3, m_cv4, m_cv5;
  int m_progress;
  bool m_check;
};

int main() {
  Program program;

  thread t1(&Program::loopWork, &program);
  thread t2(&Program::loopCheckPoints, &program);

  t1.join();
  t2.join();

  return 0;
}

[Another unrelated](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice). — Enlico, Sep 25 '20 at 06:34

score 1 · Answer 1 · answered Sep 25 '20 at 06:36

Where is flaw in my implementation of this logic?

cv.notify_one does not require, that the code after cv.wait(mlock, bind(&Program::isWorkReady, this)); continues immediatly, so it is perfectly valid that multiple checkPoint are exectued, before the code continues after cv.wait.

But after you the cv.wait you set m_check = false; to false, so if there is no further checkPoint execution remaining, that will set m_check = true;, your work function becomes stuck.

Instead of m_check being a bool you could think about making it a counter, that is incremented in checkPoint and decremented in work.

Thank you for reply. I understand now why its not working like I wanted. I ended up implementing solution like Ted suggested, it felt more intuitive - using m_check to toggle between threads — Jeekim, Sep 25 '20 at 19:15

score 1 · Accepted Answer · answered Sep 25 '20 at 07:25

The loopCheckPoints() thread holds a lock for some time, sets m_check then releases the lock and immediately goes on to grab the lock again. The loopWork() thread may not have woken up in between to react to the m_check change.

Never hold locks for long times. Be as quick as possible. If you can't get the program to work without adding sleeps, you have a problem.

One way to fix this would be to check that the worker has actually set m_check back to false:

    void work(condition_variable& cv) {
        { // lock scope
            unique_lock<mutex> mlock(m_mutex);
            cv.wait(mlock, [this] { return m_check; });
            m_progress++;
            cout << " ... " << m_progress * 20 << "%" << endl;
            m_check = false;
        }
        // there's no need to hold the lock when notifying 
        cv.notify_one(); // notify that we set it back to false
    }

    void checkPoint(condition_variable& cv) {
        // if you are going to sleep, do it without holding the lock
        // this_thread::sleep_for(chrono::milliseconds(300));

        { // lock scope
            lock_guard<mutex> guard(m_mutex);
            cout << "<Checking> " << m_progress << endl;
            m_check = true;
        }
        cv.notify_one(); // no need to hold the lock here
        {
            // Check that m_check is set back to false
            unique_lock<mutex> mlock(m_mutex);
            cv.wait(mlock, [this] { return not m_check; });
        }
    }

Thank you very much! I gained better understanding of conditional variables PLUS I've never used additional curly bracers for creating scope before! This seems very good, at least for this situation. — Jeekim, Sep 25 '20 at 17:46
@Jeekim You are welcome! I rarely use the curly brackets like this and usually create a function to provide the scope - but when dealing with very short functions with mutexes I tend to think it's easier to read, understand and maintain. Threads are complicated enough :-) — Ted Lyngmo, Sep 25 '20 at 17:57

Multiple Conditional Variables : threads out of sync problem

2 Answers2