C++11 - can't awake a thread using std::thread and std::condition_variable

Question

I'm stuck on a problem when trying to awake a thread by another one. A simple producer / consumer thing.

Below the code. Line 85 is the point I don't understand why it's not working. The producer thread fills up a std::queue and calls std::condition_variable.notify_one() while the consumer thread is waiting for NOT std::queue.empty().

Thanks in advance for any help

#include <mutex>
#include <condition_variable>
#include <queue>
#include <string>
#include <iostream>
#include <thread>

// request
class request :
    public std::mutex,
      public std::condition_variable,
      public std::queue<std::string>
{
public:
  virtual ~request();
};

request::~request()
{
}

// producer
class producer
{
public:
  producer(request &);

  virtual ~producer();

  void operator()();

private:
  request & request_;
};

producer::producer(request & _request)
:
request_(_request)
{
}

producer::~producer()
{
}

void
producer::operator()()
{
  while (true) {
    std::lock_guard<std::mutex> lock(request_);
    std::cout << "producer\n";
    request_.push("something");
    std::this_thread::sleep_for(std::chrono::seconds(1));
    request_.notify_one();
  }
}

class consumer
{
public:
  consumer(request &);

  virtual ~consumer();

  void operator()();

private:
  request & request_;
};

consumer::consumer(request & _request)
:
request_(_request)
{
}

consumer::~consumer()
{
}

void
consumer::operator()()
{
  while (true) {
    std::unique_lock<std::mutex> lock(request_); // <-- the problem
    std::cout << "consumer\n";
    request_.wait (
      lock, [this] {return !request_.empty();}
    );
    request_.pop();
  }
}

int
main()
{
  // request
  request request_;

  // producer
  std::thread producer_{producer(request_)};

  // consumer
  std::thread first_consumer_{consumer(request_)};
  std::thread second_consumer_{consumer(request_)};

  // join
  producer_.join();
  first_consumer_.join();
  second_consumer_.join();
}

Karyokinesis

mad inheritance – Altorilievo

why not? can you show a short example how it sould be? – Karyokinesis

std::thread_self::yield() might help after notify_xxx – Cortex

what... on... earth... How can something be a mutex, and a condition variable, and a queue, all at the same time? – Pteridophyte

@user1587451: if you need a mutex, a condition variable, and a queue, then have a class with 3 member variables. It's much clearer. Don't just try to inherit from everything. – Puss

@g-makulik - a yield there won't help; the producer object still holds the mutex, so the consumer thread will still be blocked. – Hornstone

What is the problem that you're seeing? What do you expect this code to do, and what does it actually do? – Hornstone

Note that the producer object sits in a tight loop, locking the mutex, doing some work, sleeping, then unlocking the mutex and immediately relocking it as it goes around the loop again. This is not a good formula for a multi-threaded application. If the sleep is intended to imitate data latency, it should be done outside the lock. – Hornstone

Answer 1

Fixed code below, with these changes:

• Don't derive from the mutex, condvar and queue like that, it's horrible.
• Unlock the mutex AS SOON AS POSSIBLE after adding an item to the queue, critical sections should always be as small as possible. This allows the consumers to wake up while the producer is sleeping.
• Flush cout (I used endl to do that) so the output is printed immediately, this makes it easier to see what's happening.
• Print "consumer" after waking, because that's when the consumer is consuming, otherwise you get misleading output showing when the consumer is sleeping, not when it gets work to do.

The main problem with your code was the producer never gave the consumers a chance to run. It added to the queue, slept for a second (still holding the mutex lock) then notified the condition variable (still holding the mutex), then really quickly released the mutex lock and acquired it again. Probably what you saw is that a consumer thread got the notification, tried to acquire the mutex lock, found it was still locked (by the producer thread) and so went back to sleep. The producer never released the mutex long enough for another thread to acquire it. You might have been able to get better results by adding a std::this_thread::yield() at the start of the producer loop, before locking the mutex, but algorithms that rely on yield() for correctness are generally broken (and indeed it makes no difference in my tests); it's better to fix the producer loop to give the consumers a chance to wake up and run.

Here's the working code:

#include <mutex>
#include <condition_variable>
#include <queue>
#include <string>
#include <iostream>
#include <thread>

// request
struct request
{
    std::mutex mx;
    std::condition_variable cv;
    std::queue<std::string> q;
};

// producer
class producer
{
public:
  producer(request & r) : request_(r) { }

  void operator()();

private:
  request & request_;
};

void
producer::operator()()
{
    while (true) {
        {
            std::lock_guard<std::mutex> lock(request_.mx);
            std::cout << "producer" << std::endl;
            request_.q.push("something");
        }
        std::this_thread::sleep_for(std::chrono::seconds(1));
        request_.cv.notify_one();
    }
}

class consumer
{
public:
  consumer(request & r) : request_(r) { }

  void operator()();

private:
  request & request_;
};

void
consumer::operator()()
{
  while (true) {
    std::unique_lock<std::mutex> lock(request_.mx);
    request_.cv.wait (
      lock, [this] {return !request_.q.empty();}
    );
    std::cout << "consumer" << std::endl;
    request_.q.pop();
  }
}

int
main()
{
  // request
  request request_;

  // producer
  std::thread producer_{producer(request_)};

  // consumer
  std::thread first_consumer_{consumer(request_)};
  std::thread second_consumer_{consumer(request_)};

  // join
  producer_.join();
  first_consumer_.join();
  second_consumer_.join();
}

Starlin

Thanks a ton for that detailed desription. I understand my failures. – Karyokinesis

I also believe the producer ought to first notify_one here and then sleep. Why keep the others waiting while you sleep? – Maffa

@Nikos, I just kept what the OP had there. I agree it probably doesn't make sense. – Starlin

Answer 2

You have to unlock your std::unique_lock before calling notify_one() otherwise your while loop will try to lock twice in the same thread. This is valid for both producer and consumer.

However, I agree with those that say that your derivation on request is very misleading. Your should be using composition. If you give me 10 mins, I might come up with something that works :)

Gauge

There's no need to unlock before notifying. There is, however, a need to unlock somewhere so that the other thread gets a chance at the mutex. – Hornstone

On top, what the @Karyokinesis wants to do is here already: en.cppreference.com/w/cpp/thread/condition_variable – Gauge

@Gauge for information, this example produces UB #15734387 – Altorilievo

@Altorilievo that example was changed since then (although it's still not really exemplary) – Menderes

@Altorilievo thanks for your link, you are absolutely right, that piece of code is broken I wrote it this way (which I think is correct): github.com/davideanastasia/coding-exercises/blob/master/… – Gauge