C++ mutex-like class using std::condition_variable

Question

On a project I'm working on, I have a need for a mutual-exclusion primitive behaving like std::mutex or std::recursive_mutex, except that it needs to accept locking and unlocking from different threads.

This is a very special and unique case in the software; I'm not trying to replace std::mutex in general. But there's one very specific place where I need this, and the current code cannot be refactored.

I don't want to use a semaphore for that purpose since, if the program crashes while the semaphore is acquired, it will fail to be acquired again until a system reboot.

So I implemented two different classes, both using std::condition_variable. The first one acts like std::mutex and the second one acts like std::recursive_mutex. Both can be used with std::lock_guard and both can be locked and unlocked from different threads.

The complete code is available on GitHub:

• https://github.com/macmade/CVLock/blob/main/CVLock/CVLock.hpp
• https://github.com/macmade/CVLock/blob/main/CVLock/CVRecursiveLock.hpp

Ignoring the fact that locking and unlocking a mutex from different threads is usually not a good idea, do you see any issue with the following code?

class CVLock
{
    public:

        CVLock(): _locked( false )
        {}

        CVLock( const CVLock & )              = delete;
        CVLock & operator =( const CVLock & ) = delete;

        void lock()
        {
            wait:

            {
                std::unique_lock l( this->_mtx );

                this->_cv.wait
                (
                    l,
                    [ this ]
                    {
                        return this->_locked == false;
                    }
                );
            }

            if( this->_mtx.try_lock() )
            {
                std::lock_guard l( this->_mtx, std::adopt_lock );

                if( this->_locked )
                {
                    goto wait;
                }
                
                this->_locked = true;
            }
            else
            {
                goto wait;
            }
        }

        void unlock()
        {
            {
                std::lock_guard l( this->_mtx );

                this->_locked = false;
            }

            this->_cv.notify_all();
        }

    private:

        bool                    _locked;
        std::mutex              _mtx;
        std::condition_variable _cv;
};

class CVRecursiveLock
{
    public:

        CVRecursiveLock(): _lockCount( 0 )
        {}

        CVRecursiveLock( const CVRecursiveLock & )              = delete;
        CVRecursiveLock & operator =( const CVRecursiveLock & ) = delete;

        void lock()
        {
            {
                std::lock_guard l( this->_mtx );

                if( this->_owner.has_value() && std::this_thread::get_id() == this->_owner.value() )
                {
                    this->_lockCount++;
                    
                    return;
                }
            }

            wait:

            {
                std::unique_lock l( this->_mtx );

                this->_cv.wait
                (
                    l,
                    [ this ]
                    {
                        return this->_lockCount == 0;
                    }
                );
            }

            if( this->_mtx.try_lock() )
            {
                std::lock_guard l( this->_mtx, std::adopt_lock );

                if( this->_lockCount != 0 )
                {
                    goto wait;
                }

                this->_lockCount += 1;
                this->_owner      = std::this_thread::get_id();
            }
            else
            {
                goto wait;
            }
        }

        void unlock()
        {
            {
                std::lock_guard l( this->_mtx );

                if( this->_lockCount != 0 )
                {
                    this->_lockCount -= 1;
                }

                if( this->_lockCount == 0 )
                {
                    this->_owner.reset();
                }
            }

            this->_cv.notify_all();
        }

    private:

        uint64_t                         _lockCount;
        std::mutex                       _mtx;
        std::condition_variable          _cv;
        std::optional< std::thread::id > _owner;
};
```

Answer 1

Unnecessary use of this->

You almost never have to write this-> in C++, including inside lambdas that capture this. I recommend you remove all use of it, as it just adds noise.

Simplify lock()

I don't see why you complicated lock() so much. I would just write:

void lock() {
    std::unique_lock l(_mtx);
    _cv.wait(l, [this] {
        return _locked == false;
    });
    _locked = true;
}

After all, once wait() returns, you are holding the lock, you can just set _locked to true without any issue. You can do something similar for the recursive mutex.

Use std::atomic_flag if you can use C++20

std::atomic_flag existed since C++11, and was a way to atomically set and clear a flag. The drawback was that you could not wait for the flag to get cleared, you either had to use it as a spinlock, or add your own mutex and condition variable, in which case it was not more efficient than your solution. However, since C++20 it has wait() and notify_one() member functions. So you can write your class like:

class CVLock {
public:
   void lock() {
       while(_locked.test_and_set()) {
           _locked.wait(true);
       }
   }

   void unlock() {
       _locked.clear();
       _locked.notify_one();
   }

private:
   std::atomic_flag _locked; // Initialized automatically since C++20
};

For the recursive mutex, you can't use std::atomic_flag, however std::atomic<T> also has wait() and notify_one() since C++20. However, storing a counter and a std::optional<std::thread::id> in T will probably negate any benefits this has.

There is no need to delete the copy constructor and assignment operator

If a class has a member variable that is not copyable, then by default the compiler will make the whole class non-copyable as well. Thus, you don't have to explicitly delete the copy constructor and copy assignment operator yourself.