CODEWITHSUNDEEP
c++multithreadingc++11thread-safetylocking
Rajs123
Rajs123

Reputation: 663

Thread Safe Queue Deadlocks

I have written a thread safe queue which gives deadlock error. I am unable to figure out reason. I modified the functions to use local locks, instead of member-variable lock. Then, it seems to run fine.

Code:

template <typename T>
class MyQueue {
queue<T> arr;

mutex mtx;
unique_lock<mutex> lck;
condition_variable cv;
public:
MyQueue() {
    lck = unique_lock<mutex>(mtx, defer_lock);
}

void push(int tmp) {
    lck.lock();
    arr.push(tmp);
    lck.unlock();
    cv.notify_one();
}

int pop() {
    T x;
    lck.lock();
    while(arr.size() == 0)
        cv.wait(lck);
    x = arr.front();
    arr.pop();
    lck.unlock();
    return x;
}

int getCount() {
    T x;
    lck.lock();
    x = arr.size();
    lck.unlock();

    return x;
}
};

Error:

libc++abi.dylib: libc++abi.dylib: libc++abi.dylib: terminating with
uncaught exception of type std::__1::system_error: unique_lock::lock: already locked:
Resource deadlock avoidedterminating with uncaught exception of type std::__1::system_error: 
unique_lock::lock: already locked: Resource deadlock avoidedlibc++abi.dylib: 
terminating with uncaught exception of type std::__1::system_error: unique_lock::lock: already locked: Resource deadlock avoided

Upvotes: 0

Views: 654

Answers (2)

Jonathan Lee
Jonathan Lee

Reputation: 517

Consider deleting the unique_lock inside class, and changing your push and pop function to the following:

void push(int tmp)
{
    std::lock_guard<std::mutex> lkg(mtx); // Here unique_lock not necessary.
    arr.push(tmp);
    cv.notify_one();
}

int pop()
{
    std::unique_lock<std::mutex> ulk(mtx);
    cv.wait(ulk, [this]() { return arr.size() != 0; });

    auto x = arr.front();
    arr.pop();
    return x;
}

The reasons are very well explained in the comments :-).

You might also want to change your mtx to mutable if you want to provide const member functions like empty(); or copy constructor.

Upvotes: 0

BadZen
BadZen

Reputation: 4265

As per my comment: unique_lock, as the name implies, is intended for use by only /one/ locking thread. To lock from another thread, you need another lock. The upshot of this - make unique_lock a local in each function, and not a class member. 

template <typename T>
class MyQueue {
queue<T> arr;

mutex mtx;
condition_variable cv;
public:
MyQueue() {
}

void push(int tmp) {
    unique_lock<mutex> lck(mtx);
    arr.push(tmp);
    cv.notify_one();
    lck.unlock(); // Not nec'y, but polite...
}

...

and so forth.

Upvotes: 1

Related Questions