Return to Answer

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!

The constructor of upgrade_lock never locks the mutex

When constructing an upgrade_lock variable, the constructor sets m_state = initial_state before calling either lock_shared() or lock_unique(). In the latter two functions, it checks the current value of m_state, and if it already matches the target state, these functions immediately return.

lock_upgrade() is only safe if mutexes are only accessed via upgrade_locks

The function lock_upgrade() has a while loop that waits for readers to become zero. The assumption being that no new read locks can be acquired after the call to disable_read(). However, that only works if the only access to a mutex is via the upgrade_lock() class. If someone calls mutex.lock_shared() directly between the end of the while-loop and before upgrade() is called, then the read lock will be successfully acquired.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.