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.

Description of the Readers–writers problem.

It looks like it works, but I have some doubts:

class ReadWriteBuffer<T> {

    private static final Object writeLock = new Object();

    private static final Object readWriteLock = new Object();

    private int numberReaders = 0;

    private int numberWriters = 0;

    private int numberWriteRequests = 0;

    public void write(T object) throws InterruptedException {

        numberWriteRequests++;

        synchronized (readWriteLock) {
            while (numberReaders > 0) {
                readWriteLock.wait();
            }
        }

        synchronized (writeLock) {

            numberWriteRequests--;

            numberWriters++;

            System.out.println("Writer #" + Thread.currentThread().getId() + " started writing.");
            Thread.sleep(3000);
            System.out.println("Writer #" + Thread.currentThread().getId() + " finished writing.");

            numberWriters--;

            synchronized (readWriteLock) {
                readWriteLock.notifyAll();
            }
        }
    }

    public void read() throws InterruptedException {

        synchronized (readWriteLock) {
            while (numberWriters > 0 || numberWriteRequests > 0) {
                readWriteLock.wait();
            }
        }

        numberReaders++;

        System.out.println("Reader #" + Thread.currentThread().getId() + " started reading.");
        Thread.sleep(1000);
        System.out.println("Reader #" + Thread.currentThread().getId() + " finished reading.");

        numberReaders--;

        synchronized (readWriteLock) {
            readWriteLock.notifyAll();
        }
    }
}

class Reader implements Runnable {

    private final ReadWriteBuffer<Object> buffer;

    private int priority;

    public Reader(int priority, ReadWriteBuffer<Object> buffer) {
        this.priority = priority;
        this.buffer = buffer;
    }

    @Override
    public void run() {
        while (true) {
            try {
                Thread.sleep(priority);
                buffer.read();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }
}

class Writter implements Runnable {

    private final ReadWriteBuffer<Object> buffer;

    private int priority;

    public Writter(int priority, ReadWriteBuffer<Object> buffer) {
        this.priority = priority;
        this.buffer = buffer;
    }

    @Override
    public void run() {
        while (true) {
            try {
                Thread.sleep(priority);
                buffer.write("Object");
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }
}

public class ReadersWriters {

    public static void main(String[] args) {

        ReadWriteBuffer<Object> sharedBuffer = new ReadWriteBuffer<Object>();

        new Thread(new Reader(1, sharedBuffer)).start();
        new Thread(new Reader(700, sharedBuffer)).start();
        new Thread(new Reader(100, sharedBuffer)).start();
        new Thread(new Writter(2000, sharedBuffer)).start();
        new Thread(new Writter(4000, sharedBuffer)).start();
    }
}
share|improve this question
    
What are your doubts? –  Jamal Aug 13 at 18:05
    
f.e. not sure about counters, seems that they should be atomic –  barn.gumbl Aug 13 at 18:07

1 Answer 1

up vote 7 down vote accepted

This code in the read() method is broken:

public void read() throws InterruptedException {

    synchronized (readWriteLock) {
        while (numberWriters > 0 || numberWriteRequests > 0) {
            readWriteLock.wait();
        }
    }

    numberReaders++;

    System.out.println("Reader #" + Thread.currentThread().getId() + " started reading.");
    Thread.sleep(1000);
    System.out.println("Reader #" + Thread.currentThread().getId() + " finished reading.");

    numberReaders--;

    synchronized (readWriteLock) {
        readWriteLock.notifyAll();
    }
}

Since numberReaders ++ and -- is not in a synchronized block, it essentially may as well not happen as far as other threads are concerned, or, worse, it could partially happen (like the ++ may happen, and the -- may not).

Additionally, they may not produce the right results, anyway. The ++ operator is not atomic, and it is possible for the value to be ++ in two different threads at the same time, and the result to be just one increment for both invocations.

Bottom line is that, from a multi-threaded perspective, this code is broken.

References:

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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