Locking C++11 std::unique_lock causes deadlock exception

Question

I'm trying to use a C++11 std::condition_variable, but when I try to lock the unique_lock associated with it from a second thread I get an exception "Resource deadlock avoided". The thread that created it can lock and unlock it, but not the second thread, even though I'm pretty sure the unique_lock shouldn't be locked already at the point the second thread tries to lock it.

FWIW I'm using gcc 4.8.1 in Linux with -std=gnu++11.

I've written a wrapper class around the condition_variable, unique_lock and mutex, so nothing else in my code has direct access to them. Note the use of std::defer_lock, I already fell in to that trap :-).

class Cond {
private:
    std::condition_variable cCond;
    std::mutex cMutex;
    std::unique_lock<std::mutex> cULock;
public:
    Cond() : cULock(cMutex, std::defer_lock)
    {}

    void wait()
    {
        std::ostringstream id;
        id << std::this_thread::get_id();
        H_LOG_D("Cond %p waiting in thread %s", this, id.str().c_str());
        cCond.wait(cULock);
        H_LOG_D("Cond %p woke up in thread %s", this, id.str().c_str());
    }

    // Returns false on timeout
    bool waitTimeout(unsigned int ms)
    {
        std::ostringstream id;
        id << std::this_thread::get_id();
        H_LOG_D("Cond %p waiting (timed) in thread %s", this, id.str().c_str());
        bool result = cCond.wait_for(cULock, std::chrono::milliseconds(ms))
                == std::cv_status::no_timeout;
        H_LOG_D("Cond %p woke up in thread %s", this, id.str().c_str());
        return result;
    }

    void notify()
    {
        cCond.notify_one();
    }

    void notifyAll()
    {
        cCond.notify_all();
    }

    void lock()
    {
        std::ostringstream id;
        id << std::this_thread::get_id();
        H_LOG_D("Locking Cond %p in thread %s", this, id.str().c_str());
        cULock.lock();
    }

    void release()
    {
        std::ostringstream id;
        id << std::this_thread::get_id();
        H_LOG_D("Releasing Cond %p in thread %s", this, id.str().c_str());
        cULock.unlock();
    }
};

My main thread creates a RenderContext, which has a thread associated with it. From the main thread's point of view, it uses the Cond to signal the rendering thread to perform an action and can also wait on the COnd for the rendering thread to complete that action. The rendering thread waits on the Cond for the main thread to send rendering requests, and uses the same Cond to tell the main thread it's completed an action if necessary. The error I'm getting occurs when the rendering thread tries to lock the Cond to check/wait for render requests, at which point it shouldn't be locked at all (because the main thread is waiting on it), let alone by the same thread. Here's the output:

DEBUG: Created window
DEBUG: OpenGL 3.0 Mesa 9.1.4, GLSL 1.30
DEBUG: setScreen locking from thread 140564696819520
DEBUG: Locking Cond 0x13ec1e0 in thread 140564696819520
DEBUG: Releasing Cond 0x13ec1e0 in thread 140564696819520
DEBUG: Entering GLFW main loop
DEBUG: requestRender locking from thread 140564696819520
DEBUG: Locking Cond 0x13ec1e0 in thread 140564696819520
DEBUG: requestRender waiting
DEBUG: Cond 0x13ec1e0 waiting in thread 140564696819520
DEBUG: Running thread 'RenderThread' with id 140564575180544
DEBUG: render thread::run locking from thread 140564575180544
DEBUG: Locking Cond 0x13ec1e0 in thread 140564575180544
terminate called after throwing an instance of 'std::system_error'
  what():  Resource deadlock avoided

To be honest I don't really understand what a unique_lock is for and why condition_variable needs one instead of using a mutex directly, so that's probably the cause of the problem. I can't find a good explanation of it online.

Answer 1

Foreword: An important thing to understand with condition variables is that they can be subject to random, spurious wake ups. In other words, a CV can exit from wait() without anyone having called notify_*() first. Unfortunately there is no way to distinguish such a spurious wake up from a legitimate one, so the only solution is to have an additional resource (at the very least a boolean) so that you can tell whether the wake up condition is actually met.

This additional resource should be guarded by a mutex too, usually the very same you use as a companion for the CV.

---

The typical usage of a CV/mutex pair is as follows:

std::mutex mutex;
std::condition_variable cv;
Resource resource;

void produce() {
    // note how the lock only protects the resource, not the notify() call
    // in practice this makes little difference, you just get to release the
    // lock a bit earlier which slightly improves concurrency
    {
        std::lock_guard<std::mutex> lock(mutex); // use the lightweight lock_guard
        make_ready(resource);
    }
    // the point is: notify_*() don't require a locked mutex
    cv.notify_one(); // or notify_all()
}

void consume() {
    std::unique_lock<std::mutex> lock(mutex);
    while (!is_ready(resource))
        cv.wait(lock);
    // note how the lock still protects the resource, in order to exclude other threads
    use(resource);
}

Compared to your code, notice how several threads can call produce()/consume() simultaneously without worrying about a shared unique_lock: the only shared things are mutex/cv/resource and each thread gets its own unique_lock that forces the thread to wait its turn if the mutex is already locked by something else.

As you can see, the resource can't really be separated from the CV/mutex pair, which is why I said in a comment that your wrapper class wasn't really fitting IMHO, since it indeed tries to separate them.

The usual approach is not to make a wrapper for the CV/mutex pair as you tried to, but for the whole CV/mutex/resource trio. Eg. a thread-safe message queue where the consumer threads will wait on the CV until the queue has messages ready to be consumed.

---

If you really want to wrap just the CV/mutex pair, you should get rid of your lock()/release() methods which are unsafe (from a RAII point of view) and replace them with a single lock() method returning a unique_ptr:

std::unique_ptr<std::mutex> lock() {
    return std::unique_ptr<std::mutex>(cMutex);
}

This way you can use your Cond wrapper class in rather the same way as what I showed above:

Cond cond;
Resource resource;

void produce() {
    {
        auto lock = cond.lock();
        make_ready(resource);
    }
    cond.notify(); // or notifyAll()
}

void consume() {
    auto lock = cond.lock();
    while (!is_ready(resource))
        cond.wait(lock);
    use(resource);
}

But honestly I'm not sure it's worth the trouble: what if you want to use a recursive_mutex instead of a plain mutex? Well, you'd have to make a template out of your class so that you can choose the mutex type (or write a second class altogether, yay for code duplication). And anyway you don't gain much since you still have to write pretty much the same code in order to manage the resource. A wrapper class only for the CV/mutex pair is too thin a wrapper to be really useful IMHO. But as usual, YMMV.