Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've found on the wikipedia pseudo-code, so I've decided to try it.

That's the Wiki's code:

int readcount, writecount; (initial value = 0)
semaphore mutex_1, mutex_2, mutex_3, w, r ; (initial value = 1)

READER
  P(mutex_3);
    P(r);
      P(mutex_1);
        readcount := readcount + 1;
        if readcount = 1 then P(w);
      V(mutex_1);
    V(r);
  V(mutex_3);

  reading is performed

  P(mutex_1);
    readcount := readcount - 1;
    if readcount = 0 then V(w);
  V(mutex_1);


WRITER
  P(mutex_2);
    writecount := writecount + 1;
    if writecount = 1 then P(r);
  V(mutex_2);

  P(w);
    writing is performed
  V(w);

  P(mutex_2);
    writecount := writecount - 1;
    if writecount = 0 then V(r);
  V(mutex_2);

I have implemented that into c++ under linux.

Here's my code:

#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <unistd.h>
#include <time.h>
#include <iostream>
#include <string.h>
#include <sstream>

using namespace std;


int readcount = 0, writecount = 0;
pthread_mutex_t mutex_1, mutex_2, mutex_3, w, r;


// my own additional variables
int R, W; // read from main args ammount of reading and writing threads
int var_for_msg = 0; // should be 0 or 1 (number of writers actually in a library)
pthread_mutex_t  msg_lock; // locking message so two threads will not be able to write at the same time in the console
// ---------------------- //

void write_if_smth_changed()
{
pthread_mutex_lock(&msg_lock);  // lock msg_lock
cout<<"ReaderQ: " << R-readcount << " WriterQ: " << W - var_for_msg << " [in: R:" << readcount <<" W:"<< var_for_msg << "]" << endl;
pthread_mutex_unlock(&msg_lock); // unlock msg_lock

if (var_for_msg > 0)
{
cout<< "HAHAHAHAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" << endl;
}
}

void *f_write(void *writers_number)
{
while (true)
    {   
    pthread_mutex_lock(&mutex_2); 
    writecount++;   
    if (writecount == 1)
        {
        pthread_mutex_lock(&r);     
        }   
    pthread_mutex_unlock(&mutex_2);


    pthread_mutex_lock(&w); 


    var_for_msg++;  
    write_if_smth_changed();
    var_for_msg--;
    write_if_smth_changed();

    pthread_mutex_unlock(&w);

    pthread_mutex_lock(&mutex_2);

    writecount--;   
    if (writecount == 0)
        {       
        pthread_mutex_unlock(&r);
        }
    pthread_mutex_unlock(&mutex_2); 
    //sleep(1);
    }
}

void *f_read(void *readers_number)
{
while (true)
    {
    pthread_mutex_lock(&mutex_3);
        pthread_mutex_lock(&r);     
            pthread_mutex_lock(&mutex_1);
                readcount++;
                if (readcount == 1)
                    {                   
                    pthread_mutex_lock(&w);
                    }
            pthread_mutex_unlock(&mutex_1); 
        pthread_mutex_unlock(&r);

    pthread_mutex_unlock(&mutex_3);

    write_if_smth_changed();

    pthread_mutex_lock(&mutex_1);
    readcount--;    
    if (readcount == 0)
        {
            pthread_mutex_unlock(&w);
        }
    pthread_mutex_unlock(&mutex_1);
    //sleep(1);
    }
}


int main(int argc, char *argv[])
{

/* args input */
istringstream iss(argv[1]);
iss >> R;
istringstream iss2(argv[2]);
iss2 >> W;
/* ----------------------- */


pthread_mutex_init(&mutex_1,NULL);
pthread_mutex_init(&mutex_2,NULL);
pthread_mutex_init(&mutex_3,NULL);
pthread_mutex_init(&w,NULL);
pthread_mutex_init(&r,NULL);
pthread_mutex_init(&msg_lock,NULL);

pthread_t *thread_readers_array = new pthread_t[R];
pthread_t *thread_writers_array = new pthread_t[W];

for (int i = 0; i< R; i++)
{
pthread_create(&thread_readers_array[i],NULL,f_read,(void *)i); 
}

for (int i = 0; i< W; i++)
{
pthread_create(&thread_writers_array[i],NULL,f_write,(void *)i);    
}


pthread_join(thread_writers_array[0],NULL);
pthread_join(thread_readers_array[0],NULL); // 


return 0;
}

I've added in there a function which is supposed to write to the console the actual number of readers/writers in Queue and readers/writers inside library.
e.g.:

ReaderQ: 618 WriterQ: 5 [in: R: 382 W:0] would mean: 618 readers and 5 writers in queue, 382 readers and 0 writers inside library.

When I launch the executable file with args 1000 and 5 (1000 reading and 5 writing threads)

./a.out 1000 5

It should once in a while output HAHAHAHA~ (because there's a writer in the room) however it's not happening. Could this be possible that the algo presented in a wikipedia is wrong? If not, where's the problem?

share|improve this question

closed as off topic by Jeff Vanzella, Brian Reichle, Glenn Rogers, w0lf, palacsint Jan 9 '13 at 6:39

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

    
The wikipedia article looks correct. But your code is relatiely convoluted and you perform no checks to see if any of the lock/unlock calls work as expected that there could be any number of problems. –  Loki Astari Jan 2 '13 at 20:36
    
If a function returns an error code and you do not check the state of the error code your code will at some point break. –  Loki Astari Jan 2 '13 at 20:38
add comment

1 Answer 1

This is not how you would write the code in C++ (this is C (and awful C at that)).

I would implement the class:

 class RWLock
 {
      friend class ReadLocker;
      friend class WriteLocker;

      void getReadLock();
      void unLockRead();

      void getWriteLock();
      void unLockWrite();


      private:
          // All the members you need here
      public:
          RWLock(): /*Initialize all members explicitly*/ {}
         ~RWLock() {/* TIDY UP */}
 };
 class ReadLocker
 {
      RWLock& lock;
      public:
          ReadLocker(RWLock& lock) : lock(lock) {lock.getReadLock();}
         ~ReadLocker()                          {lock.unlockRead();}
 };
 class WriteLocker
 {
      RWLock& lock;
      public:
          WriteLocker(RWLock& lock) : lock(lock) {lock.getWriteLock();}
         ~WriteLocker()                          {lock.unlockWrite();}
 };

Then usage would be:

 void Reader()
 {
     // Reader
     ReadLocker    lock(sourceLock);    // gets a read lock
     // do reading here
 }

 void Writer()
 {
     // Writter
     WriteLock     lock(sourceLock);   // gets a write lock.
     // do writting here
 }

Now all you have to implement are four methods the usage of the class is short and intuitive. And most importantly the user of the RWLock can not use it incorrectly and leave the locks in the incorrect state.

So now lets implement the getXXXLock()

Notice that we use RAII to call Lock/Unlock on any lockable objects in an exception safe manor. For this we use MutexLocker which will call lock() in the constructor of the object and unlock() in the destructor when it goes out of scope. In the few cases were we can not use RAII directly we still guarantee that the calls will be made in pairs because we guarantee that for each call to getXXXLock() we will also call unlockXXX().

void RWLock::getReadLock()
{
    MutexLocker lock3(mutex_3);
    MutexLocker lockR(r);
    MutexLocker lock1(mutex_1);

    ++readcount;
    if (readcount)
    {   Lock(w);   // This is OK
    }              // because we guarantee that 
                   // unlockRead() is called thus making sure we call
                   // the unlock() as a controlled pair.
}  

// reading is performed

void RWLock::unlockRead()
{
     MutexLocker lock1(mutex_1);
    --readcount;
     if (readcount == 0)
     {    unlock(w);     // See getReadLock()
     }
}


void RWLock::getWriteLock()
{
    {
        MutexLocker  lock2(mutex_2);
        ++writecount;
        if (writecount == 1)
        {   Lock(r);          // See lock(w) below
        }
    }

    lock(w);      // This is OK because we guarantee to call unlockWrite()
                  // Which is the counterpart to this call.
}

// writing is performed

void RWLock::unlockWrite()
{
    unlock(w);

    MutexLocker lock2(mutex_2);
    --writecount;
    if (writecount == 0)
    {   unlock(r);
    }
}
share|improve this answer
add comment

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