Deadlock simulation using std::mutex

Question

I have following example:

template <typename T>
class container
{
public:
    std::mutex _lock;
    std::set<T> _elements;

    void add(T element)
    {
        _elements.insert(element);
    }

    void remove(T element)
    {
        _elements.erase(element);
    }
};

void exchange(container<int>& cont1, container<int>& cont2, int value)
    {
        cont1._lock.lock();
        std::this_thread::sleep_for(std::chrono::seconds(1));

        cont2._lock.lock();

        cont1.remove(value);
        cont2.add(value);

        cont1._lock.unlock();
        cont2._lock.unlock();
    }

    int main() 
    {
        container<int> cont1, cont2;

        cont1.add(1);
        cont2.add(2);

        std::thread t1(exchange, std::ref(cont1), std::ref(cont2), 1);
        std::thread t2(exchange, std::ref(cont2), std::ref(cont1), 2);

        t1.join();
        t2.join();

        return 0;
    }

In this case I'm expiriencing a deadlock. But when I use std::lock_guard instead of manually locking and unlocking mutextes I have no deadlock. Why?

void exchange(container<int>& cont1, container<int>& cont2, int value)
{
    std::lock_guard<std::mutex>(cont1._lock);
    std::this_thread::sleep_for(std::chrono::seconds(1));

    std::lock_guard<std::mutex>(cont2._lock);

    cont1.remove(value);
    cont2.add(value);
}

Answer 1

Your two code snippets are not comparable. The second snippet locks and immediately unlocks each mutex as the temporary lock_guard object is destroyed at the semicolon:

std::lock_guard<std::mutex>(cont1._lock);  // temporary object

The correct way to use lock guards is to make scoped variables of them:

{
    std::lock_guard<std::mutex> lock(my_mutex);

    // critical section here

}   // end of critical section, "lock" is destroyed, calling mutex.unlock()

(Note that there is another common error that's similar but different:

std::mutex mu;
// ...
std::lock_guard(mu);

This declares a variable named mu (just like int(n);). However, this code is ill-formed because std::lock_guard does not have a default constructor. But it would compile with, say, std::unique_lock, and it also would not end up locking anything.)

Now to address the real problem: How do you lock multiple mutexes at once in consistent order? It may not be feasible to agree on a single lock order across an entire codebase, or even across a future user's codebase, or even in local cases as your example shows. In such cases, use the std::lock algorithm:

std::mutex mu1;
std::mutex mu2;

void f()
{
    std::lock(mu1, mu2);

    // order below does not matter
    std::lock_guard<std::mutex> lock1(mu1, std::adopt_lock);        
    std::lock_guard<std::mutex> lock2(mu2, std::adopt_lock);
}

In C++17 there is a new variadic lock guard template called scoped_lock:

void f_17()
{
    std::scoped_lock lock(mu1, mu2);

    // ...
}

The constructor of scoped_lock uses the same algorithm as std::lock, so the two can be used compatibly.

Answer 2

While Kerrek SB's answer is entirely valid I thought I'd throw an alternative hat in the ring. std::lock or any try-and-retreat deadlock avoidance strategies should be seen as the last resort from a performance perspective.

How about:

#include <functional> //includes std::less<T> template.

static const std::less<void*> l;//comparison object. See note.

void exchange(container<int>& cont1, container<int>& cont2, int value)
    {
        if(&cont1==&cont2) {
            return; //aliasing protection.
        }
        std::unique_lock<std::mutex> lock1(cont1._lock, std::defer_lock);
        std::unique_lock<std::mutex> lock2(cont2._lock, std::defer_lock);
        if(l(&cont1,&cont2)){//in effect portal &cont1<&cont2
            lock1.lock();
            std::this_thread::sleep_for(std::chrono::seconds(1));
            lock2.lock();
        }else{
            lock2.lock();
            std::this_thread::sleep_for(std::chrono::seconds(1));
            lock1.lock();
        } 
        cont1.remove(value);
        cont2.add(value);
    }

This code uses the memory address of the objects to determine an arbitrary but consistent lock order. This approach can (of course) be generalized.

Note also that in reusable code the aliasing protection is necessary because the version where cont1 is cont2 would be invalid by trying to lock the same lock twice. std::mutex cannot be assumed to be a recursive lock and normally isn't.

NB: The use of std::less<void> ensures compliance as it guarantees a consistent total ordering of addresses. Technically (&cont1<&cont2) is unspecified behavior. Thanks Kerrek SB!