Mutexes and deadlocks

Question

After asking a question about mutexes here, I was warned about deadlocks.

Would the example I put together below be a reasonable way to avoid deadlocks?

class Foo
{
public:
    Foo();

    void Thread();

    int GetWidgetProperty();
    int GetGadgetProperty();

private:

    Widget widget_;
    Gadget gadget_;

    VDK::MutexID widgetLock;
    VDK::MutexID gadgetLock;
};


Foo::Foo()
    : widget_(42)
    , gadget_(widget_)
{
    widgetLock = VDK::CreateMutex();
    gadgetLock = VDK::CreateMutex();
}

void Foo::Thread()
{
    while(1)
    {
        VDK::AcquireMutex(widgetLock);
        // Use widget
        VDK::ReleaseMutex(widgetLock);

        VDK::AcquireMutex(widgetLock);
        VDK::AcquireMutex(gadgetLock);
        // use gadget
        VDK::ReleaseMutex(widgetLock);
        VDK::ReleaseMutex(gadgetLock);
    }
}

int Foo::GetWidgetProperty()
{
    VDK::AcquireMutex(widgetLock);
    return widget_.GetProp();
    VDK::ReleaseMutex(widgetLock);
}

int Foo::GetGadgetProperty()
{
    VDK::AcquireMutex(widgetLock);
    VDK::AcquireMutex(gadgetLock);
    return gadget.GetProp();
    VDK::ReleaseMutex(widgetLock);
    VDK::ReleaseMutex(gadgetLock);  
}

Since the call GetGadgetProperty could result in using the widget, I'm guessing we also need to protect our self with a lock here. My question is, am I requiring and releasing them in the right order?

I don't understand your code. Are you trying to implement your own concurrency primitives? Is there any reason you are not using C++11's thread support library? — 5gon12eder, Aug 04 '15 at 12:53
I'm developing for an Embedded system that does not support C++11. VDK is a kernel from Analog Devices. — Q-bertsuit, Aug 04 '15 at 13:08
My approach in embedded programming was to avoid lock-fests like this as much as possible. I prefer Actor approach. Your active C++ object (one with a thread inside) receives messages and acts on them, then sends messages as needed. This way, the only locking you usually need to get right is related to the message queue. — BitTickler, Aug 04 '15 at 13:45
Put your efforts on always locking in the same order. This [SO post](http://stackoverflow.com/a/6012781/5037799) is well detailed and it's exactly what you are looking for. — Richard Dally, Aug 04 '15 at 12:50

score 7 · Accepted Answer · answered Aug 04 '15 at 12:55

Your code has obvious deadlock. You can't release mutex after return statement. Whats more it's better to unlock them in reverse (to locking) order. Proper code should look like this:

int Foo::GetWidgetProperty()
{
    VDK::AcquireMutex(widgetLock);
    int ret = widget_.GetProp();
    VDK::ReleaseMutex(widgetLock);
    return ret;
}

int Foo::GetGadgetProperty()
{
    VDK::AcquireMutex(widgetLock);
    VDK::AcquireMutex(gadgetLock);
    int ret = gadget.GetProp();
    VDK::ReleaseMutex(gadgetLock);  
    VDK::ReleaseMutex(widgetLock);
    return ret;
}

JBL · Answer 2 · 2015-08-04T13:04:20.650

2

An even better way would be to rely on RAII to do the job for you.

I invite you to read about std::lock_guard. The basic principle is that you acquire the mutex by declaring an object. And the mutex is released automatically at the end of its lifetime.

Now, you can use block scopes for the region of code that needs to lock a mutex that way:

{
    std::lock_guard lockWidget{widgetMutex};//Acquire widget mutex
    std::lock_guard lockGadget{gadgetMutex};//Acquire gadget mutex
    //do stuff with widget/gadget
    //...
    //As the lock_guards go out of scope in the reverse order of 
    //declaration, the mutexes are released
}

Of course, this works with the standard library mutexes, so you'd have to adapt to your use.

That would prevent error such as trying to release a mutex after a return statement, which obviously will never happen, or in the face of exception that would happen before you actually release the mutex.

edited Aug 04 '15 at 13:04

answered Aug 04 '15 at 12:55

JBL

12,588
4
53
84

1

`std::lock_guard` is useful. But he's asking for best practices in order to avoid dead locks. – Richard Dally Aug 04 '15 at 12:56
@LeFlou Well, they are part of the best practices for that. They notably avoid potential deadlocks in the face of exceptions. Moreover, it avoids human errors as you don't have to pair every lock with an unlock. – JBL Aug 04 '15 at 12:57
2

You can't possibly talk about best practices without mentioning RAII, which actually solves both the problem and other problems likely to come up anyway. +1 – Barry Aug 04 '15 at 13:04

Mutexes and deadlocks

2 Answers2