7
\$\begingroup\$

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:

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;
};
```
\$\endgroup\$
1
  • \$\begingroup\$ You should provide usecase. Otherwise the class sounds inherently very unsafe. Basically, there is no promise that after locking the mutex, it is actually locked as a different thread can unlock it and another thread could lock it and start modifying causing data race. If you can tell what you actually need it for them perhaps someone can give a proper advise of what class to use instead. \$\endgroup\$
    – ALX23z
    Commented Feb 24, 2023 at 16:01

1 Answer 1

5
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the answer. This is a C++17 codebase, so unfortunately I won't be able to use std::atomic_flag. But nice to know C++20 added wait. \$\endgroup\$
    – Macmade
    Commented Feb 23, 2023 at 23:27
  • \$\begingroup\$ About the simplification of lock, you're completely right. I think I had an issue with the initial implementation and I ended up making things more complicated. \$\endgroup\$
    – Macmade
    Commented Feb 23, 2023 at 23:29
  • 1
    \$\begingroup\$ Minor simplification - the cv.wait() test can just return !_locked rather than comparing with false. Could possible use a test-and-set there (or std::exchange()), so as not to need the subsequent _locked = true assignment. Thanks for telling us about the addition of wait/notify methods in C++20 - I missed that as it went past! \$\endgroup\$ Commented Feb 24, 2023 at 7:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.