lock_upgrade()
is not thread-safe
Imagine two threads calling lock_upgrade()
at the same time. Both threads think there are only readers at that point, so they both set the state to rip_parallel::upgrade_state
, they both decrement m_readers
, then increment m_upgraders
, m_mutex.readers_count()
will return 0
for both threads, and then they both call m_mutex.upgrade_lock()
. Now you have two threads thinking they have exclusive access. It doesn't matter that m_readers
and m_upgraders
are atomic here!
Don't use verbs for class and variable names
You named your class upgrade_mutex
. This sounds like an action. It is best to use nouns for class and variable names. In this case, you could use upgradable_mutex
, or better yet, upgradable_rwlock
.
I see you copied Boost's naming convention here, but I would say that in this specific case, they made a bad choice.
Mirror C++11 mutex classes wherever possible
Make your class look and behave existing mutex classes as much as possible. It's the principle of least surprise for the users of your class. For example, C++11 mutexes are held by a lock_guard<>
, which again is a noun. Change the class upgrade_lock
accordingly, or even better, make a template class lock_guard<>
inside the rip_parallel
namespace that can work with your locks.
Also, lock_guard<>
locks the mutex by default, and this is what users who already know regular mutexes expect. So make the default initial_state = rip_parallel::shared_state
.
Make internal functions private
or protected
Your class has functions like wait_read()
which, I assume, are not meant to be used directly by application code. So they should be hidden. Also, some functions in theupgrade_mutex
class, like upgrade()
, are misnamed; they don't really upgrade the lock, they only implement part of the functionality needed for upgrading it.
Move as much logic as possible to the mutex class
The upgrade_lock
class is there to provide RAII-style locking. The only thing it should do is lock in the constructor, unlock in the destructor, and pass any useful function like upgrading and downgrading to the mutex.
Move constant initialization of member variables out of the constructors
Instead of making the constructor like this:
upgrade_mutex(void): m_readers(0), m_upgraders(0) { ... }
Move the initialization of m_readers
and m_upgraders
to their declaration:
private:
volatile std::atomic<int> m_readers = 0;
volatile std::atomic<int> m_upgraders = 0;
The benefit of this becomes clear when you are writing classes with more than one constructor, or if you have many variables to initialize in your class.