Vtyjkyuk

For answering this question of stackoverflow I have written the code
https://stackoverflow.com/questions/12033188/how-would-you-implement-your-own-reader-writer-lock-in-c11

Can someone review it - so that I can understand the possible problems in the code.

#include <condition_variable>
#include <iostream>
#include <shared_mutex>
#include <thread>
#include <unistd.h>
#define NR_THREADS 10
#include <mutex>

class MySharedLock 
 public:
 void read_lock() 
 std::unique_lock<std::mutex> lk(rw_mutex);
 std::cout << "nReader Lock Writers are " << writers << std::flush;
 if (writers != 0) 
 rw_cv.wait(lk, [this]() return (this->writers == 0); );
 
 readers++;
 lk.unlock();
 

 void write_lock() 
 std::unique_lock<std::mutex> lk(rw_mutex);

 std::cout << "nWriter Lock Writers are " << writers << " Readers are "
 << readers << std::flush;
 if (readers == 0 && writers == 0) 
 std::cout << "nWriter Lock Writers are " << writers << std::flush;
 else 
 rw_cv.wait(
 lk, [this]() return (this->writers == 0 && this->readers == 0); );
 
 writers++;
 lk.unlock();

 

 void write_unlock() 
 std::lock_guard<std::mutex> lk(rw_mutex);
 writers--;
 rw_cv.notify_all();
 

 void read_unlock() 
 std::lock_guard<std::mutex> lk(rw_mutex);
 if (readers == 1) // I am the last one.
 rw_cv.notify_all();
 
 readers--;
 

 explicit MySharedLock() 

 private:
 std::mutex rw_mutex;
 std::condition_variable rw_cv;
 uintmax_t readers = 0, writers = 0;
;

First, I suppose it would be cheating to just use a std::shared_mutex, right? :)

Second, it seems a little weird to declare uintmax_t writers for a variable that can only ever hold the value 0 or 1. But, on the other hand, the symmetry between readers and writers is kind of nice.

uintmax_t readers = 0, writers = 0;

Strongly prefer to declare one variable per line. Also, I know some people like to write int i0;, and of course the natural thing to write is int i = 0;, but your belt-and-suspenders approach is relatively unusual. Prefer

uintmax_t readers = 0;
uintmax_t writers = 0; // or, bool has_writer = false;

explicit MySharedLock() 

I am a big fan of this explicit zero-argument constructor! However, you should probably be in the habit of =defaulting things instead of .

if (writers != 0) 
 rw_cv.wait(lk, [this]() return (this->writers == 0); );

Nit: A lambda that doesn't escape should generally just capture [&]. And you needn't write out this-> if you don't want to. So:

if (writers != 0) 
 rw_cv.wait(lk, [&]() return (writers == 0); );

However, this could be more cleanly expressed as simply

while (writers != 0) 
 rw_cv.wait(lk);

lk.unlock();

unique_lock is an RAII type; it unlocks the controlled mutex automatically in its destructor. You don't need to (and therefore shouldn't) write .unlock explicitly in your code.

This implementation has the potential problem that it delegates all of the "queueing" logic to the mutex/cv themselves. It is possible that if customers show up as "reader, writer, reader, writer, reader, writer...", then the readers could starve out the writers, or even vice versa. The only way to fix this, AFAIK, is to implement some kind of a "queue" of waiters, so that each waiter knows exactly when it's at the head of the queue. (Imagine the ticketing system in a deli: "Now serving number 47!")

Inside reader_unlock, you have rw_cv.notify_all() — but I think that this could safely be changed to rw_cv.notify_one(). We know that nobody is waiting to read; therefore all waiters are writers; therefore at most one of them will actually be able to do any work right now; therefore it is sufficient to wake up just one of them.

(And of course by the time that guy wakes up, somebody new may have come in and stolen the lock. So even that guy might not get to do any work.)

However, what notify_all does do is transfer over all the waiters from waiting on the rw_cv to waiting on the rw_mutex. If std::mutex implements some kind of fair queueing system, this ensures that no writer will get starved for longer than one epoch-of-readers, because as soon as the epoch ends, all the writers will get woken up and transferred over to the mutex, so that their respective epochs-of-writers will happen bam bam bam one after the other.

Yeah, I think I've convinced myself that notify_all is actually the most effective thing to do here. But it probably deserves a code comment explaining why you think it's the right thing to do!

Finally, it's worth mentioning that the standard names for your methods (according to the standard SharedMutex concept) are:

read_lock -> lock_shared
read_unlock -> unlock_shared
write_lock -> lock
write_unlock -> unlock

There's no standard C++ name for the promotion of a held reader lock to a writer lock (which is deceptively tricky), nor for the downgrading of a held writer lock to a reader lock (which is trivial). You might think about whether you want to provide these operations, and if so, what the interface to them should be.