4
\$\begingroup\$

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};
};
\$\endgroup\$
6
\$\begingroup\$

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 i{0};, 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.

| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ If writers are starving out readers, then a reader/writer lock isn't the right tool for the job, so I wouldn't worry too much about that case. // If fairness is needed for writers, then the proposed queue solution works fine, but otherwise a pending_writers counter would also work. \$\endgroup\$ – hoffmale Aug 13 '18 at 9:53
-4
\$\begingroup\$

here is my implementation

class RWLock {
public:
    virtual void rlock() = 0;
    virtual void runlock() = 0;
    virtual void wlock() = 0;
    virtual void wunlock() = 0;
};

/**
 * a reader preferred lock
 */
class RWLock1: public RWLock{
public:
    void rlock() override {
        unique_lock<mutex> guard(m_reader_lock);
        ++m_count;
        if (m_count == 1) {
            m_res_lock.lock();
        }
    }

    void runlock() override {
        unique_lock<mutex> guard(m_reader_lock);
        --m_count;
        if (m_count == 0) {
            m_res_lock.unlock();
        }
    }

    void wlock() override {
        m_res_lock.lock();
    }

    void wunlock() override {
        m_res_lock.unlock();
    }

private:
    mutex m_res_lock; // resource lock
    mutex m_reader_lock; // reader count lock
    int m_count = 0; // reader count
};

// a relatively fair rwlock
class RWLock2: public RWLock {
public:
    void rlock() override {
        lock_guard<mutex> ser_guard(m_ser_lock);
        lock_guard<mutex> read_guard(m_reader_lock);
        ++m_read_count;
        if (m_read_count == 1) {
            m_res_lock.lock();
        }
    }

    void runlock() override {
        lock_guard<mutex> read_guard(m_reader_lock);
        --m_read_count;
        if (m_read_count == 0) {
            m_res_lock.unlock();
        }
    }

    void wlock() override {
        lock_guard<mutex> ser_guard(m_ser_lock);
        m_res_lock.lock();
    }

    void wunlock() override {
        m_res_lock.unlock();
    }
private:
    mutex m_res_lock; // resource lock
    mutex m_ser_lock; // 服务锁,只有拿到服务锁才能被服务, 只有开始服务才释放锁
    mutex m_reader_lock; // reader count lock
    int m_read_count = 0; // 读计数
};

void run_rwlock_test(int r_count, int rw_ratio, RWLock &lock) {
    ASSERT_TRUE(rw_ratio > 0);
    vector<int> data;
    vector<thread> ths;
    for (int i = 0; i < r_count; ++i) {
        // add reader
        ths.emplace_back(thread([&, i=i]{
            cout << "reader [" << i << "] started" << endl;
            lock.rlock();
            cout << "reader [" << i << "] reading: " << data.size() << endl;
            this_thread::sleep_for(chrono::milliseconds(1));
            lock.runlock();
            cout << "reader [" << i << "] done" << endl;
        }));

        // add writer
        if (i % rw_ratio == 0) {
            ths.emplace_back(thread([&, i=i/rw_ratio]{
                cout << "writer [" << i << "] started" << endl;
                lock.wlock();
                data.push_back(i);
                cout << "writer [" << i << "] writing: " << data.size() << endl;
                this_thread::sleep_for(chrono::milliseconds(1));
                lock.wunlock();
                cout << "writer [" << i << "] done" << endl;
            }));
        }
    }

    for (auto &th: ths) {
        th.join();
    }

    ASSERT_EQ(data.size(), r_count / rw_ratio);
}
```
| improve this answer | |
\$\endgroup\$
  • 4
    \$\begingroup\$ You present another implementation. This does not constitute a valid code review. \$\endgroup\$ – greybeard Sep 1 at 7:31
  • 1
    \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process. \$\endgroup\$ – Vogel612 Sep 1 at 12:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.