Unlock of unowned mutex

Question

I created the following class which provides an acquire_lock() and release_lock() function

class LockableObject {

public:

    void acquire_lock() {
        std::unique_lock<std::mutex> local_lock(m_mutex);
        m_lock = std::move(local_lock);
    }

    void release_lock() {
        m_lock.unlock();
    }

private:

    std::mutex m_mutex;

    std::unique_lock<std::mutex> m_lock;
};

This class provides an acquire_lock and release_lock function. I have multiple threads accessing the same object and calling the acquire_lock before performing any operations and then calling release_lock once done as below.

void ThreadFunc(int ID, LockableObject* lckbleObj)
{
    for (int i = 0; i < 1000; i++)
    {
        lckbleObj->acquire_lock();
        std::cout << "Thread with ID = " << ID << "doing work" << std::endl;
        std::this_thread::sleep_for(std::chrono::milliseconds(10));
        lckbleObj->release_lock();
    }
}



void main()
{
    const int numThreads = 10;
    std::thread workerThreads[numThreads];
    LockableObject *testObject = new LockableObject();

    for (int i = 0; i < numThreads; i++)
    {
        workerThreads[i] = std::thread(ThreadFunc, i, testObject);
    }

    for (int i = 0; i < numThreads; i++)
    {
        workerThreads[i].join();
    }
}

In the acquire_lock function, I first try to lock the underlying mutex (m_mutex) with a local stack std::unique_lock object by passing it ( m_mutex) in the constructor. I assume once the constructor for std::unique_lock returns it has locked the mutex, I then move the unique_lock on the stack to the member variable m_lock.

This program is flawed in some basic way and during the call to release_lock will result in the "unlock of unowned mutex", I seem to be missing something basic about std::unique_lock and am looking for someone to correct my understanding.

Answer 1

See my comment about the lack of std::defer_lock in the constructor. But you also have a race condition in your code.

The acquire_lock function modifies the m_lock under protection of the m_mutex mutex. Thus, to ensure thread safety, no other thread can modify m_lock except while holding m_mutex.

But the release_lock function modifies m_lock while it is releasing that mutex. Thus, you do not have proper synchronization on m_lock.

This is somewhat subtle to understand. This is the problem code:

    m_lock.unlock();

Note that when this function is entered, m_mutex is locked but during its execution, it both modifies m_lock and releases m_mutex in no particular guaranteed order. But m_mutex protects m_lock. So this is a race condition and not permitted.

It can be fixed as follows:

void release_lock() {
    std::unique_lock<std::mutex> local_lock = std::move(m_lock);
    local_lock.unlock();
}

Now, this first line of code modifies m_lock but runs entirely with m_mutex held. This avoids the race condition.

The unlock can be removed if desired. The destructor of local_lock will do it.

By the way, I would suggest changing the API. Instead of offering lock and unlock calls, instead have a way to create an object that owns a lock on this object. You can even use std::unique_lock<LockableObject> if you want. Why create a new API that's worse than the one offered by the standard?