C++ implementation of recursive_shared_mutex

Question

I know it's bad to even use recursive_mutex because of its poor performance, let alone a recursive shared mutex. However, I'm doing this just to practice. Any suggestion will be appreciated!

recursive_shared_mutex.hpp

#pragma once #include <optional> #include <shared_mutex> #include <thread> class recursive_shared_mutex : private std::shared_mutex { public: recursive_shared_mutex(); ~recursive_shared_mutex() = default; void lock(); bool try_lock(); void unlock(); /* * std::shared_mutex's shared locking is recursive by default. */ void lock_shared() { std::shared_mutex::lock_shared(); } bool try_lock_shared() { return std::shared_mutex::try_lock_shared(); } void unlock_shared() { std::shared_mutex::unlock_shared(); } private: std::mutex mtx_; std::condition_variable cv_; std::optional<std::thread::id> writer_id_; std::size_t writer_cnt_; }; 

recursive_shared_mutex.cpp

#include "recursive_shared_mutex.hpp" recursive_shared_mutex::recursive_shared_mutex() : std::shared_mutex(), mtx_(), cv_(), writer_id_(), writer_cnt_(0) { } void recursive_shared_mutex::lock() { std::thread::id this_id = std::this_thread::get_id(); std::unique_lock<std::mutex> ulock(mtx_); if (this_id == writer_id_) { // Same thread/writer trying to acquire the shared_mutex again. // Simply increase writer count. ++writer_cnt_; } else { // Another writer or no writer is holding the shared_mutex. // It's also likely that some readers are holding // the shared_mutex. if (writer_id_.has_value()) { // If another writer is holding the mutex, // waiting for it to release. cv_.wait(ulock, [this] { return writer_cnt_ == 0; }); } std::shared_mutex::lock(); writer_id_ = this_id; writer_cnt_ = 1; } } bool recursive_shared_mutex::try_lock() { // TODO return false; } void recursive_shared_mutex::unlock() { std::unique_lock<std::mutex> ulock(mtx_); if (!writer_id_.has_value()) { // No writer is holding the shared_mutex. // Call unlock() anyway. UB expected. std::shared_mutex::unlock(); } else { // At this point, the shared_mutex must be held by a writer // and writer_cnt_ must be greater than 0. --writer_cnt_; if (writer_cnt_ == 0) { // If writer count becomes 0, release the // underlying shared_mutex. // It's likely that we are unlocking in a // different thread from the one where the shared_mutex // has been acquired. // Call unlock() anyway. UB expected. std::shared_mutex::unlock(); writer_id_.reset(); // Manually unlock to let cv notify. ulock.unlock(); cv_.notify_one(); } } } 

Answer 1

I strongly recommend not inheriting from standard types, and not using private inheritance even with your own types. If a recursive_shared_mutex IS-A shared_mutex, then announce it publicly; if it IS-NOT-A shared_mutex, then don't use inheritance at all!

(Godbolt.D_view here is isomorphic to std::reference_wrapper<D>, for what that's worth.)

---

std::size_t writer_cnt_; 

should be

std::size_t writer_cnt_ = 0; 

so that you don't have to remember to initialize it in the member-initializer-list of the constructor. Once you do that, you can =default your constructor. Even if you don't =default the constructor, please don't do mtx_(), cv_(), and so on. Let default constructors do their jobs; and don't force me to read more code than I need to read.

---

 // No writer is holding the shared_mutex. // Call unlock() anyway. UB expected. 

This comment is just completely bogus. If this situation is "should never happen, UB expected," then assert false already! Don't just go on and do some random operation like shared_mutex::unlock() when you know you're in a bad state. Assert false, and let the programmer detect and fix their bug.

Again, this follows the mantra "Don't force me to read more code than I have to." Don't make me read a codepath that does shared_mutex::unlock() and returns, when really that codepath is unreachable. In fact, don't even write that if test! Just do this:

void recursive_shared_mutex::unlock() { auto lk = std::unique_lock<std::mutex>(mtx_); assert(writer_id_.has_value()); assert(*writer_id_ == std::this_thread::id()); assert(writer_cnt_ > 0); if (--writer_cnt_ == 0) { std::shared_mutex::unlock(); // oops, TODO FIXME writer_id_ = std::nullopt; lk.unlock(); cv_.notify_one(); } } 

Notice my preferred use of lk for lock-guard variables. Notice I'm avoiding the use of ad-hoc named methods like x.reset() in favor of value-semantic operations like x = nullopt.

You've still got a bug here, which you code-commented — but commenting a bug doesn't make it go away! Frankly, this code is literally off-topic here, because it doesn't work and you know it doesn't work.

Figure out how to use a semaphore (from the <semaphore> header in C++20) or an atomic, instead of the shared_mutex which you already know gives you UB.

---

I'm 90% sure that there's a deadlock possible between your exclusive lock() and unlock() functions. Suppose Thread A has the mutex exclusive-locked, and Thread B goes to lock it. Then Thread B will enter lock(), take an exclusive lock on mtx_, and block in std::shared_mutex::lock() because Thread B still has it locked. (Assume Thread B drops its lock just long enough to fool the cv_.wait() loop.) Finally, Thread A enters unlock() and blocks in ulock(mtx_) because Thread B still has mtx_ locked.