Skip to main content
2 of 4
added 224 characters in body
G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152

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.

G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152