Ensure that a thread doesn't lock a mutex twice?

Question

Say I have a thread running a member method like runController in the example below:

class SomeClass {
public:
    SomeClass() { 
         // Start controller thread
         mControllerThread = std::thread(&SomeClass::runController, this) 
    }

    ~SomeClass() {
         // Stop controller thread
         mIsControllerThreadInterrupted = true;
         // wait for thread to die.
         std::unique_lock<std:::mutex> lk(mControllerThreadAlive); 
    }

    // Both controller and external client threads might call this
    void modifyObject() {
         std::unique_lock<std::mutex> lock(mObjectMutex);
         mObject.doSomeModification();
    }
    //...
private:
    std::mutex mObjectMutex;
    Object mObject;

    std::thread mControllerThread;
    std::atomic<bool> mIsControllerInterrupted;
    std::mutex mControllerThreadAlive;

    void runController() {        
        std::unique_lock<std::mutex> aliveLock(mControllerThreadAlive);
        while(!mIsControllerInterruped) {
            // Say I need to synchronize on mObject for all of these calls
            std::unique_lock<std::mutex> lock(mObjectMutex);
            someMethodA();
            modifyObject(); // but calling modifyObject will then lock mutex twice
            someMethodC();
        }
    }
    //...
};

And some (or all) of the subroutines in runController need to modify data that is shared between threads and guarded by a mutex. Some (or all) of them, might also be called by other threads that need to modify this shared data.

With all the glory of C++11 at my disposal, how can I ensure that no thread ever locks a mutex twice?

Right now, I'm passing unique_lock references into the methods as parameters as below. But this seems clunky, difficult to maintain, potentially disastrous, etc...

void modifyObject(std::unique_lock<std::mutex>& objectLock) {

    // We don't even know if this lock manages the right mutex... 
    // so let's waste some time checking that.
    if(objectLock.mutex() != &mObjectMutex)
         throw std::logic_error();

    // Lock mutex if not locked by this thread
    bool wasObjectLockOwned = objectLock.owns_lock();
    if(!wasObjectLockOwned)
        objectLock.lock();

    mObject.doSomeModification();

    // restore previous lock state
    if(!wasObjectLockOwned)
        objectLock.unlock();

}

Thanks!

I am a little confused. Do you have unchecked `try_to_lock`s? How can a thread lock a mutex and then forget about it? — Red Alert, Apr 23 '14 at 03:25
I don't understand the nature of the problem. You lock the mutex right before reading or modifying shared data, and unlock right after. You would normally have an instance of `unique_lock` on the stack, not passed in as a parameter. You must be doing something quite unorthodox if locking the same mutex twice is an actual risk in your design. Can you show exactly how this may happen for you? — Igor Tandetnik, Apr 23 '14 at 03:43
@RedAlert Ah, perhaps `std::try_to_lock` is the answer I was looking for? Can I just call that from every function that needs a lock instead of creating a local `unique_lock` or passing locks around? — mxdubois, Apr 23 '14 at 03:44
@IgorTandetnik I updated the question with a more explicit example, but let me know if it's still unclear. — mxdubois, Apr 23 '14 at 04:04
@mxdubois no, then you'd lose mutual exclusion with other threads. You need to resolve this at the design level. In your example, a good fix would be to completely remove mutexes from `modifyObject()`, and instead have only the functions that call it handle the access to the shared resource. — Red Alert, Apr 23 '14 at 04:24
Use a recursive mutex and avoid the whole problem. Just use 'Resource Acquisition Is Initialization' to acquire/release the `unique_lock` as an object on the stack. — user207421, Apr 23 '14 at 04:30
@EJP [this thread](http://stackoverflow.com/q/187761/1599617) leads me to believe that it's cleaner to avoid recursive mutexes if possible. Thanks for the suggestion though :) — mxdubois, Apr 23 '14 at 07:26
What's the point of `mControllerThreadAlive`? Do you have something against [`std::thread::join`](http://en.cppreference.com/w/cpp/thread/thread/join)? — Casey, Apr 23 '14 at 21:24
@Casey Oh yeah... it doesn't make sense in the example, but in my project I want to be able to check if the thread is still alive without ending it. — mxdubois, Apr 23 '14 at 23:52

nosid · Accepted Answer · 2014-04-23T22:42:55.377

There are several ways to avoid this kind of programming error. I recommend doing it on a class design level:

separate between public and private member functions,
only public member functions lock the mutex,
and public member functions are never called by other member functions.

If a function is needed both internally and externally, create two variants of the function, and delegate from one to the other:

public:
    // intended to be used from the outside
    int foobar(int x, int y)
    {
         std::unique_lock<std::mutex> lock(mControllerThreadAlive);
         return _foobar(x, y);
    }
private:
    // intended to be used from other (public or private) member functions
    int _foobar(int x, int y)
    {
        // ... code that requires locking
    }

And then there are class private methods that you bind to `this` and pass along as callbacks. Then you don't know whether that method should be prefixed by an underscore as all the other private methods, or not. It gets messy. — schlimmchen, Apr 20 '20 at 19:08

Ensure that a thread doesn't lock a mutex twice?

1 Answers1