Implementing a resource read/write lock in Java

I’m trying to implement a simple read/write lock for a resource accessed concurrently by multiple threads. The workers randomly try reading or writing to a shared object. When a read lock is set, workers should not be able to write until the lock is released. When a write lock is set, read and write are not permitted. Although my implementation seems to work, I believe it is conceptually wrong.

A read operation taking place should allow for more read operations happening at the same time, resulting in the overall number of reads being larger than the number of writes. My program yields numbers that follow the probability of these operations being performed by a worker.

I feel like my implementation is actually not concurrent at all, but I’m having a hard time identifying the mistake. I would really appreciate being pointed in the right direction.

Main class that dispatches and terminates workers:

class Main {

    private static final int THREAD_NUMBER = 4;

    public static void main(String[] args) {
        // creating workers
        Thread[] workers = new Thread[THREAD_NUMBER];
        for (int i = 0; i < THREAD_NUMBER; i++) {
            workers[i] = new Thread(new Worker(i + 1));
        }
        System.out.println("Spawned workers: " + THREAD_NUMBER);

        // starting workers
        for (Thread t : workers) {
            t.start();
        }
        try {
            Thread.sleep((long) 10000);
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
        }

        // stopping workers
        System.out.println("Stopping workers...");
        for (Thread t : workers) {
            t.interrupt();
        }
    }
}

The Resource class:

class Resource {

    enum ResourceLock {
        ON,
        OFF
    } 

    private static Resource instance = null;
    private ResourceLock writeLock = ResourceLock.OFF;
    private ResourceLock readLock = ResourceLock.OFF;

    private Resource() {}

    public static synchronized Resource getInstance() {
        if (instance == null) {
            instance = new Resource();
        }
        return instance;
    }

    public ResourceLock getWriteLock() {
        return writeLock;
    }
    public ResourceLock getReadLock() {
        return readLock;
    }
    public void setWriteLock() {
        writeLock = ResourceLock.ON;
    }
    public void setReadLock() {
        readLock = ResourceLock.ON;
    }
    public void releaseWriteLock() {
        writeLock = ResourceLock.OFF;
    }
    public void releaseReadLock() {
        readLock = ResourceLock.OFF;
    }
}

And finally the Worker class:

import java.util.Random;

class Worker implements Runnable {

    private static final double WRITE_PROB = 0.5;
    private static Random rand = new Random();
    private Resource res;
    private int id;

    public Worker(int id) {
        res = Resource.getInstance();
        this.id = id;
    }

    public void run() {
        message("Started.");
        while (!Thread.currentThread().isInterrupted()) {
            performAction();
        }
    }

    private void message(String msg) {
        System.out.println("Worker " + id + ": " + msg);
    }

    private void read() {
        synchronized(res) {
            while (res.getWriteLock() == Resource.ResourceLock.ON) {
                try {
                    wait();
                } catch (InterruptedException e) {
                    Thread.currentThread().interrupt();
                }
            }
            res.setReadLock();
            // perform read
            try {
                Thread.sleep((long) 500);
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
            }
            res.releaseReadLock();
            res.notifyAll();
        }
        message("Finished reading.");
    }

    private void write() {
        synchronized(res) {
            while (res.getWriteLock() == Resource.ResourceLock.ON || res.getReadLock() == Resource.ResourceLock.ON) {
                try {
                    wait();
                } catch (InterruptedException e) {
                    Thread.currentThread().interrupt();
                }
            }
            res.setWriteLock();
            // perform write
            try {
                Thread.sleep((long) 500);
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
            }
            res.releaseWriteLock();
            res.notifyAll();
        }
        message("Finished writing.");
    }

    private void performAction() {
        double r = rand.nextDouble();
        if (r <= WRITE_PROB) {
            write();
        } else {
            read();
        }
    }
}

The reasoning behind having two separate locks for read and write is that I want to have the ability to atomise both operations and their queries for the lock.

Here is an example of the output I’m getting with a 0.5 write probability:

Spawned workers: 4
Worker 2: Started.
Worker 3: Started.
Worker 1: Started.
Worker 4: Started.
Worker 2: Finished writing.
Worker 4: Finished reading.
Worker 1: Finished writing.
Worker 3: Finished writing.
Worker 1: Finished reading.
Worker 4: Finished writing.
Worker 2: Finished reading.
Worker 4: Finished reading.
Worker 1: Finished reading.
Worker 3: Finished writing.
Worker 1: Finished writing.
Worker 4: Finished writing.
Worker 2: Finished writing.
Worker 4: Finished writing.
Worker 1: Finished reading.
Worker 3: Finished writing.
Worker 1: Finished writing.
Worker 4: Finished reading.
Worker 2: Finished writing.
Stopping workers...
Worker 4: Finished writing.
Worker 1: Finished writing.
Worker 3: Finished reading.
Worker 2: Finished reading.

Help much appreciated.

Answer

You are performing the entire operation within a synchronized block, so there is no concurrency. Further, there is no precedence towards any lock kind, as at most one thread can own a lock. Not performing the entire operation in a synchronized block won’t work with your current code, as every reader does a readLock = ResourceLock.OFF at the end, regardless of how many readers are there. Without a counter, you can’t support multiple readers correctly.

Besides that, it’s a strange code structure, to provide a Resource class maintaining the state but leaving it entirely up to the callers to do the right thing with it. That’s not the way to deal with responsibility and encapsulation.

An implementation may look like

class ReadWriteLock {
    static final int WRITE_LOCKED = -1, FREE = 0;

    private int numberOfReaders = FREE;
    private Thread currentWriteLockOwner;

    public synchronized void acquireReadLock() throws InterruptedException {
        while(numberOfReaders == WRITE_LOCKED) wait();
        numberOfReaders++;
    }
    public synchronized void releaseReadLock() {
        if(numberOfReaders <= 0) throw new IllegalMonitorStateException();
        numberOfReaders--;
        if(numberOfReaders == FREE) notifyAll();
    }
    public synchronized void acquireWriteLock() throws InterruptedException {
        while(numberOfReaders != FREE) wait();
        numberOfReaders = WRITE_LOCKED;
        currentWriteLockOwner = Thread.currentThread();
    }
    public synchronized void releaseWriteLock() {
        if(numberOfReaders!=WRITE_LOCKED || currentWriteLockOwner!=Thread.currentThread())
            throw new IllegalMonitorStateException();
        numberOfReaders = FREE;
        currentWriteLockOwner = null;
        notifyAll();
    }
}

It simply uses a counter of acquired read locks, setting the counter to -1 when there is a write lock (so write locks can not be nested). Acquiring a read lock may succeed whenever there is no write lock, so there is no need to implement precedence for them, the possibility to succeed when another thread already has a real lock, is sufficient. In fact, when having a significantly larger number of readers than writers, you may encounter the “starving writer” problem.

The worker simplifies to

class Worker implements Runnable {
    private static final double WRITE_PROB = 0.5;
    private static final Random rand = new Random();
    private final ReadWriteLock theLock;
    private final int id;

    public Worker(int id, ReadWriteLock lock) {
        theLock = lock;
        this.id = id;
    }

    public void run() {
        message("Started.");
        while(!Thread.currentThread().isInterrupted()) {
            performAction();
        }
    }

    private void message(String msg) {
        System.out.println("Worker " + id + ": " + msg);
    }

    private void read() {
        try {
            theLock.acquireReadLock();
        } catch(InterruptedException e) {
            Thread.currentThread().interrupt();
            return;
        }
        // perform read
        try {
            Thread.sleep(500);
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
        }
        finally { theLock.releaseReadLock(); }
        message("Finished reading.");
    }

    private void write() {
        try {
            theLock.acquireWriteLock();
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
            return;
        }
        // perform write
        try {
            Thread.sleep(500);
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
        }
        finally { theLock.releaseWriteLock(); }
        message("Finished writing.");
    }

    private void performAction() {
        double r = rand.nextDouble();
        if (r <= WRITE_PROB) {
            write();
        } else {
            read();
        }
    }
}

Note that I avoided global variables here. The lock should get passed to the constructor. It’s also important that the methods return when being interrupted during the lock acquisition. Self interrupting and retrying the acquisition like in your original code will lead to an infinite loop, as the next wait would again throw an InterruptedException after you restored the current thread’s interrupted state. Of course, proceeding without having the lock would be wrong too, so the only valid options are not restoring the interrupted state or returning immediately.

The only change to your main program is to construct a pass the lock instance:

ReadWriteLock sharedLock = new ReadWriteLock();
// creating workers
Thread[] workers = new Thread[THREAD_NUMBER];
for (int i = 0; i < THREAD_NUMBER; i++) {
    workers[i] = new Thread(new Worker(i + 1, sharedLock));
}
System.out.println("Spawned workers: " + THREAD_NUMBER);

// starting workers
for (Thread t : workers) {
    t.start();
}
try {
    Thread.sleep(10000);
} catch (InterruptedException e) {
    Thread.currentThread().interrupt();
}

// stopping workers
System.out.println("Stopping workers...");
for (Thread t : workers) {
    t.interrupt();
}