Readers writers problem concurrent Java

Question

This is an implementation of readers writers, i.e. many readers can read but only one writer can write at any one time. Does this work as expected?

public class ReadersWriters extends Thread{

static int num_readers = 0;
static int writing = 0;

public void read_start() throws InterruptedException {         

    synchronized(this.getClass()) {
        while(writing == 1) wait();
        num_readers++;
    }        
}

public void read_end() {
    synchronized(this.getClass()) {
        if(--num_readers == 0) notifyAll();
    }
}

public void write_start() throws InterruptedException{

    synchronized(this.getClass()) {
        while(num_readers > 0) wait();
        writing = 1;
    } 
}

public void write_end() {
    this.getClass().notifyAll();
}
}

Also is this implementation any different from declaring each method

public static synchronized read_start()

for example?

Thanks

in my opinion you should avoid waiting/notifying on an object that is publicly available, as this.class or this is. This allows for roque code screwing up your synchronization. The synchronizing object should be a private field of this class. — MeBigFatGuy, May 22 '11 at 18:39
Is this homework? If not @Alex Gitelman's answer should likely be accepted. — Greg Mattes, May 22 '11 at 19:07
> Does this work as expected? (a) what do you expect? (b) what does it do? (c) is there a difference? — user207421, May 23 '11 at 02:21

score 6 · Accepted Answer · edited Aug 04 '13 at 17:19

No - you're implicitly calling this.wait(), despite not having synchronized on this, but instead on the class. Likewise you're calling this.notifyAll() in read_end. My suggestions:

Don't extend Thread - you're not specializing the thread at all.
Don't use static variables like this from instance members; it makes it look like there's state on a per-object basis, but actually there isn't. Personally I'd just use instance variables.
Don't use underscores in names - the conventional Java names would be numReaders, readEnd (or better, endRead) etc.
Don't synchronize on either this or the class if you can help it. Personally I prefer to have a private final Object variable to lock on (and wait etc). That way you know that only your code can be synchronizing on it, which makes it easier to reason about.
You never set writing to 0. Any reason for using an integer instead of a boolean in the first place?

Of course, it's better to use the classes in the framework for this if at all possible - but I'm hoping you're really writing this to understand threading better.

@Ferguzz: No - you're still not setting `writing` to 0 (in `write_end`) and it's still confusing to make *instance* methods completely regardless of their instance (other than to get the class). See my list of bullet points to fix. — Jon Skeet, May 22 '11 at 18:44

score 3 · Answer 2 · answered May 22 '11 at 18:42

You can achieve your goal in much simpler way by using

java.util.concurrent.locks.ReentrantReadWriteLock

Just grab java.util.concurrent.locks.ReentrantReadWriteLock.ReadLock when you start reading and java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock when you start writing.

This class is intended exactly for that - allow multiple readers that are mutually exclusive with single writer.

score 2 · Answer 3 · answered May 22 '11 at 18:40

Your particular implementation of read_start is not equivalent to simply declaring the method synchronized. As was noted by J. Skeed, you need to call notify (and wait) on the object you are synchronize-ing with. You cannot use an unrelated object (here: the class) for this. And using the synchronized modified on a method does not make the method implicitly call wait or anything like that.

There is, BTW., an implementation of read/write locks, which ships with the core JDK: java.util.concurrent.locks.ReentrantReadWriteLock. Using that one, your code might look like the following instead:

class Resource {
    private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
    private final Lock rlock = lock.readLock();
    private final Lock wlock = lock.writeLock();

    void read() { ... /* caller has to hold the read lock */ ... }
    void write() { ... /* caller has to hold the write lock */ ... }

    Lock readLock() { return rlock; }
    Lock writeLock() { return wlock; }
}

Usage

final Resource r = ...;

r.readLock().lock();
try {
    r.read();
} finally {
    r.unlock();
}

and similar for the write operation.

score 0 · Answer 4 · answered May 22 '11 at 18:59

The example code synchronizes on this.getClass(), which will return the same Class object for multiple instances of ReadersWriters in the same class loader. If multiple instances of ReadersWriters exist, even though you have multiple threads, there will be contention for this shared lock. This would be similar to adding the static keyword to a private lock field (as Jon Skeet suggested) and would likely lead to worse performance than synchronizing on this or a private lock object. More specifically, one thread which is reading would be blocking another thread which is writing, and this is likely undesirable.

that is the required behaviour. Either one thread can write on its own. or many threads can read when no thread is writing. — Ferguzz, May 22 '11 at 19:01

Readers writers problem concurrent Java

4 Answers4

Linked