Consumer producer multithreading freezes

Question

I'm trying to create a simple consumer/producer code for learning, in which a producer pushes numbers into a stack, and consumer threads print the numbers, heres what I got:

const int N_THREADS = 10;
const int N_TESTS = 100;
bool finished = false;
queue<int> q;

void produce()
{
    for (int i = 0; i < N_TESTS; i++)
    {
        q.push(i);
        cv.notify_all();        
    }
    finished = true;
}

void consume()
{
    while (!q.empty() || !finished)
    {   
        unique_lock<mutex> lock(m);
        cv.wait(lock, [] {return !q.empty(); });
        int i = q.front();
        cout << i << endl;
        q.pop();
    }   
}

int main()
{
    //thread that will be used for producing
    thread producer(produce);
    
    //vector of consumer threadss
    vector<thread> consumers(N_THREADS);
    for (int i = 0; i < N_THREADS; i++)
    {
        consumers[i] = thread(consume);
    }

    //joining all threads
    producer.join();
    for (int i = 0; i < N_THREADS; i++)
    {
        consumers[i].join();
    }
    
    return 0;   
}

However, when i run the code it prints the numbers but it just freezes, it never ends:

What can be done for it to end?

Doesn't look like the producer is guarding itself against simultaneous access of the queue. — user4581301, May 28 '21 at 05:25
`while (return !q.empty() || !finished)` this is legal? I have legitimately never seen this before... — Borgleader, May 28 '21 at 05:25
`std::queue` is not a thread-safe data structrure. You may not using it from multiple threads at once without synchronization. — Daniel Langr, May 28 '21 at 05:26
@user4581301 How could i guard itself against those simultaneous access? — cheveuxdelin, May 28 '21 at 05:27
@cheveuxdelin By using it only inside critical sections protected by a mutex, for example. Alternatively, you can use some thread-safe (possibly lock-free) queue, but these are not provided by C++ itself. — Daniel Langr, May 28 '21 at 05:28
@Borgleader was a typo, I've edited it but it wasn't the problem anyway — cheveuxdelin, May 28 '21 at 05:30
@Borgleader compiler spits a warning, but seems to eat it. But is a `return` statement convertible to `bool`? Smurfed if I know. Never tried it. Correction: Compiler HATED it. Hate-hate-hate. — user4581301, May 28 '21 at 05:30
here is thing when you code finished processing 100 item it may block at `cv.wait()` and wait for notification and because your code finished and dont send notification any more(from producer side) it will block at `cv.wait()` and in consequence it will not check your while condition. — N0ll_Boy, May 28 '21 at 05:35
@N0ll_Boy you're onto something there. You should formalize that thought into an answer. — user4581301, May 28 '21 at 05:38

N0ll_Boy · Answer 1 · 2021-05-28T06:29:10.507

i spotted some bugs on your code.

first why you call notify_all() you pushed just one element i think it's better idea to call notify_one().

imagine this senario: producer push one element then call notify_all() then others threads wakeup one of them will take mutex and do work and pop element and then other thread check condition and see queue is empty and then they will go to sleep(waste of resource).

another important bug is that when producer finished his work it will return at this moment may queue still has element in this case some thread will wait for notification but since producer exited and therefore producer will not send notification anymore and it will cause some threads block at cv.wai(). even it's notify condition variable it will block because condition of !q.empty() return false(because q.empty() is already true after producer finished).

in order to fix this code you should modify some part of your code.

first i recommend you to also acquire mutex when you are pushing to queue because it is possible that you face race condition(in here you are using queue i dont think it will happened here but if you use vector you will definitely see).

because code block at cv.wait()(but beware some thread will exit because they will notified before producer finished and after producer finished they are going to check while condition and they see finished variable is true therefore they have chance to exit but other thread will block at cv.wait()). in order to fix this problem you should put another condition in your condition variable:

cv.wait(lock, [] {return !q.empty() || (finished && q.empty()); });
if (finished && q.empty())
        break;

and your code exit successfully.

i also recommend you to read this topics:

C++11 Can I ensure a condition_variable.wait() won't miss a notification?

https://www.modernescpp.com/index.php/c-core-guidelines-be-aware-of-the-traps-of-condition-variables

273K · Answer 2 · 2021-05-28T16:32:03.577

What is cv and m in your code? I may guess them, but Minimal, Reproducible Example would be better.

q.empty() and finished in consume is accessed without the guard. Move unique_lock<mutex> lock(m) before that access. The condition in the while loop looks wrong. Should not it be while (!finished)?

void consume()
{
    unique_lock<mutex> lock(m);
    while (!finished)
    {   
        cv.wait(lock, [] {return !q.empty(); });
        int i = q.front();
        cout << i << endl;
        q.pop();
    }   
}

produce can notify listeners when there is no one. You don't lock the queue before modifying.

void produce()
{
    unique_lock<mutex> lock(m);
    for (int i = 0; i < N_TESTS; i++)
    {
        q.push(i);
        lock.unlock();
        cv.notify_all();        
        lock.lock();
    }
    finished = true;
}

Consumer producer multithreading freezes

2 Answers2

Linked