c++multithreadingrace-conditioncondition-variablemutual-exclusion

Why is There a Deadlock Occurring Randomly in a Producer/Consumer Multithreaded C++ Program?


In the code shown below, we were tasked with creating a consumer/producer program using multithreading and to find a way to prevent deadlock. This is my code:

#include <iostream>
#include <stack>
#include <time.h>
#include <mutex>
#include <condition_variable>
#include <random>
#include <thread>
#include <windows.h>
#include <algorithm>
#include <stack>

#define MAX 100
#define MIN 0

std::condition_variable cv;
std::mutex mtx;

class ProducerConsumer{

    public:

        // Creates a stack
        std::stack<int> stack_array;

        // Facilitates checking of values
        int producer_sum = 0;
        int consumer_sum = 0;

        // Generates random number
        int random_number_generator(){
            std::random_device rd;
            std::default_random_engine re{rd()};

            return re();
        }

        // Produces values and pushes them into the stack       
        void producer(){
            for (int i = 0 ; i < MAX ; i++){

                std::unique_lock<std::mutex> lg(mtx);

                if (stack_array.size() == MAX){
                    std::cout << "Producer Thread " << std::this_thread::get_id() << " is waiting" << std::endl;
                    cv.wait(lg);
                }

                int rand_num = random_number_generator()%10+1;
                int rand_num_sleep = random_number_generator()%100+1;
                
                std::cout << "Current Stack Size -  " << stack_array.size() <<  " | Pushing: " << rand_num << "... " << "and sleeping for: " << rand_num_sleep << " milliseconds" << std::endl;

                producer_sum += rand_num;

                stack_array.push(rand_num); 

                std::this_thread::sleep_for(std::chrono::milliseconds(rand_num_sleep)); 
                
                cv.notify_all();
                lg.unlock();
            }
        }

        void consumer(){
            for (int i = 0 ; i < MAX ; i++) {

                std::unique_lock<std::mutex> lg(mtx);

                if (stack_array.size() == MIN){
                    std::cout << "Consumer Thread " << std::this_thread::get_id() << " is waiting" << std::endl;
                    cv.wait(lg);
                }

                int rand_num_sleep = random_number_generator()%100+1;
            
                std::cout << "Current Stack Size - " << stack_array.size() << " | Popping: " << stack_array.top() << "... " << "and sleeping for: " << rand_num_sleep << " milliseconds" << std::endl;

                consumer_sum += stack_array.top();

                stack_array.pop();  

                std::this_thread::sleep_for(std::chrono::milliseconds(rand_num_sleep));
                
                cv.notify_all();
                lg.unlock();
            }

        }
};

int main(){

    ProducerConsumer pc;

    std::thread producer_one (&ProducerConsumer::producer, &pc);
    std::thread consumer_one (&ProducerConsumer::consumer, &pc);
    std::thread producer_two (&ProducerConsumer::producer, &pc);
    std::thread consumer_two (&ProducerConsumer::consumer, &pc);

    producer_one.join();
    producer_two.join();
    consumer_one.join();
    consumer_two.join();

    std::cout << "Producer sum: " << pc.producer_sum << std::endl;
    std::cout << "Consumer sum: " << pc.consumer_sum << std::endl;
    std::cout << "Stack size: " << pc.stack_array.size() << std::endl;
}

When trying to run the code, there are times where the code will encounter a deadlock due to what I assume is the program starting with the producer thread (which is understandable as I have not programmed a way to circumvent that). But sometimes, the code runs perfectly fine. Other times, the program gets a segmentation fault or encounters a deadlock mid-execution.

I assume my implementation of mutual exclusion is condition variables are correct. If not, can anyone explain what is wrong with my code? Any help is appreciated. Thank you very much!


Solution

  • condition variable wait always has to be in a loop because it can wake up and another thread has already taken the item, or it can wake up for no reason at all.

    Of course the compiler won't tell you an error if there's no loop. I mean the variable is useless without the loop because it's not reliable.

    Your producers use notify_all and you have two consumers, that means after one item is added you wake up both consumers (and both producers) and they both try to get the one item. Of course one of them is not going to have an item there - unless the other producer also produced an item (by chance) in the meantime.

    Instead of:

                if (stack_array.size() == MAX){
                    std::cout << "Producer Thread " << std::this_thread::get_id() << " is waiting" << std::endl;
                    cv.wait(lg);
                }
    

    you should simply change if to while - and the same in the consumers.

    Note that looping is required even if you are careful to balance the number of notifies and consumes, because wait can return for no reason at all. On some operating systems, sometimes, wait isn't sure whether a notify happened or not, so it stops waiting anyway. This is called a "spurious wakeup". Then you check whether the queue is full and if it's still full you wait some more.