CODEWITHSUNDEEP
c++multithreadingc++11
Ramy Al Zuhouri
Ramy Al Zuhouri

Reputation: 22006

Multithreading application has no output

I am doing an application which handle multithreading for exercise.Let's suppose that we have 10 cars, and there is a parking which may contain at maximum 5 cars.If a car can't park it waits until there is a free room.
I am doing this with c++11 threads: 

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

using namespace std;

int cars=0;
int max_cars=5;
mutex cars_mux;
condition_variable cars_cond;

bool pred()
{
    return cars< max_cars;
}

void task()
{
    unique_lock<mutex> lock(cars_mux);
    while(true)
    {
        cars_mux.lock();
        cars_cond.wait(lock,pred);
        cars++;
        cout << this_thread::get_id() << " has parked" << endl;
        cars_mux.unlock();
        this_thread::sleep_for(chrono::seconds(1));  // the cars is parked and waits some time before going away
        cars_mux.lock();
        cars--;
        cars_cond.notify_one();
        cars_mux.unlock();
    }
}

int main(int argc, char** argv)
{
    thread t[10];
    for(int i=0; i<10; i++)
        t[i]=thread(task);
    for(int i=0; i<10; i++)
        t[i].join();
    return 0;
}

The problem is that there is no output, it seems like all threads are blocked waiting.

Upvotes: 4

Views: 197

Answers (2)

Bob Miller
Bob Miller

Reputation: 599

Fraser's answer is great, but when I looked at this I felt a working example would be nice.

I made some changes:

  • Personally, I like to use RAII to lock/unlock so that you don't forget to do so (even if it means extra scopes). In the past, I've also had race conditions that I couldn't figure out, and switching to the RAII approach has often made those disappear as well... It's just easier, so do it ;)

  • I like to see when the cars leave, so I add I/O for that too.

Here's the working code example for the stated problem. FYI, I use clang 3.1 with libc++:

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

using namespace std;

int cars=0;
int max_cars=5;
mutex cars_mux;
condition_variable cars_cond;

bool pred()
{
    return cars < max_cars;
}

void task()
{
    {
        unique_lock<mutex> carlock(cars_mux);
        cars_cond.wait(carlock,pred);
        cars++;
            cout << "Thread " << this_thread::get_id() 
                 << " has parked. There are " << cars << " parked cars." << endl;
    }

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

    {
        unique_lock<mutex> carlock(cars_mux);
        cars--;
        cout << "Thread " << this_thread::get_id() 
             << " has left. There are " << cars << " parked cars." << endl;
        cars_cond.notify_one();
    }
}

int main(int argc, char** argv)
{
    const int NumThreads = 10;
    thread t[NumThreads];
    for(int i=0; i<NumThreads; i++)
        t[i]=thread(task);
    for(int i=0; i<NumThreads; i++)
        t[i].join();
    return 0;
}

Edit: Simplified code as per Rami Al Zuhouri's suggestion.

Upvotes: 2

Fraser
Fraser

Reputation: 78458

There are 2 issues here:

Firstly, when you construct your lock object

unique_lock<mutex> lock(cars_mux);

then cars_mux is locked. So it is an error (undefined behaviour) to try and lock cars_mux again in the same thread, which is what you attempt to do immediately inside the while loop at

cars_mux.lock();

Secondly, there is no way for the threads to join since there is no way to exit the while(true) loop in task - cars will continue to park forever! You don't need the while loop at all.

If you remove the first cars_mux.lock();, the corresponding unlock attempt at the end of the while loop, and the while loop itself, you should get the desired behaviour.

Upvotes: 3

Related Questions