CODEWITHSUNDEEP
c++mutexdeadlock
Q-bertsuit
Q-bertsuit

Reputation: 3437

Mutexes and deadlocks

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?

Upvotes: 3

Views: 748

Answers (2)

JBL
JBL

Reputation: 12907

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.

Upvotes: 3

Glapa
Glapa

Reputation: 800

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;
}

Upvotes: 7

Related Questions