# Readers and writers (writers-preference). I'm not sure whether my code is wrong or not [closed]

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)

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

P(mutex_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;

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()
{
cout<<"ReaderQ: " << R-readcount << " WriterQ: " << W - var_for_msg << " [in: R:" << readcount <<" W:"<< var_for_msg << "]" << endl;

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

void *f_write(void *writers_number)
{
while (true)
{
writecount++;
if (writecount == 1)
{
}

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

writecount--;
if (writecount == 0)
{
}
//sleep(1);
}
}

{
while (true)
{
{
}

write_if_smth_changed();

{
}
//sleep(1);
}
}

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

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

for (int i = 0; i< R; i++)
{
}

for (int i = 0; i< W; i++)
{
}

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?

-

## closed as off topic by Jeff Vanzella, Brian Reichle, Glenn Rogers, w0lf, palacsintJan 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

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 WriteLocker;

void getWriteLock();
void unLockWrite();

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


Then usage would be:

 void Reader()
{
}

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);

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

{
MutexLocker lock1(mutex_1);
}
}

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);
}
}

-