How to correctly implement producer consumer problem-CodePudding

I tried to implement producer consumer problem in C , but I found that my implementation has a problem that it will produce until the size reaches the capacity and then start the consume process. I wonder what's the problem with my implementation.

#include <iostream>
#include <mutex>
#include <thread>
#include <queue>
#include <chrono>
using namespace std;

std::mutex mtx;
condition_variable cv;
queue<int> q;
int n=2;

int produceData() {
    int res=rand()00;
    cout<<"produce data:"<<res<<endl;
    return res;
}

void consumeData() {
    cout<<"consume data:"<<q.front()<<endl;
}
void producer(){
    while(true) {
        unique_lock<mutex> lk(mtx);
        cv.wait(lk,[&](){return q.size()<n;});
        q.push(produceData());
        cv.notify_one();
        std::this_thread::sleep_for(std::chrono::seconds(1));
    }
}

void consumer() {
    while(true) {
        unique_lock<mutex> lk(mtx);
        cv.wait(lk,[&](){return q.size()>0;});
        consumeData();
        q.pop();
        cv.notify_one();
        std::this_thread::sleep_for(std::chrono::seconds(1));
    }
}
int main() {
    std::thread t1(producer);
    std::thread t2(consumer);
    t1.join();
    t2.join();
    return 0;
}

CodePudding user response：

You need to unlock the mutex while you sleep. Just introducing a new block (curly braces) is all you need to do. Such that the unique_lock destructor will run before the sleep call. You could also manually invoke lk.unlock before the sleep call. Instead of this:

void producer(){
    while(true) {
        unique_lock<mutex> lk(mtx);
        cv.wait(lk,[&](){return q.size()<n;});
        q.push(produceData());
        cv.notify_one();
        std::this_thread::sleep_for(std::chrono::seconds(1));
    }
}

this:

void producer(){
    while(true) {

        {
            unique_lock<mutex> lk(mtx);
            cv.wait(lk,[&](){return q.size()<n;});
            q.push(produceData());
            cv.notify_one();
        } // mutex will implicitly unlock on this line as a result of the destructor for lk


        std::this_thread::sleep_for(std::chrono::seconds(1));
    }
}

Make a similar change in the consumer function as well.

CodePudding user response：

Adding to what @selbie said,

Your threads are starving each other.

Most mutex-like things in most programming languages or libraries are unfair. When one thread releases a lock while other threads are waiting for it, the lock is not automatically awarded to the thread that has been waiting the longest.

In your producer and consumer loops, as soon as either thread releases the lock, the very next thing it does is, it tries to re-lock it again. Meanwhile, the other thread has been blocked—swapped out—waiting for the lock. When the one thread releases the lock, the OS will start the process of waking up the other thread, but long before that can happen, the first thread grabs the lock again.

Moving the sleep_for() out of the critical section as @selbie suggested gives the other thread time to wake up and take the lock for itself.