5
\$\begingroup\$

I have a shared data object with writer threads and reader threads. Number of threads can read from the shared data at the same time (in the code getmethod), and writing (in my code move method) can be executed only by one thread at a time

I also have to make sure that while writing updates, only the single writer thread can access the shared data.

I'm new to java and not an expert in multithreading in general and wanted to get some comments and improvment ideas for my implementation:

SharedData:

public class SharedData {

    private int x;
    private int y;

    public SharedData(int x, int y){
        this.x= x;
        this.y = y;
    }

    public SharedData get(){
        return (new SharedData(x, y));
    }

    public void move (int dx, int dy){
        x = x+dx;
        y = y+dy;
    }

    @Override
    public String toString() {
        return x + " " + y;
    }
}

SharedDataC (Need to take care of the sync demands in this class):

import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.ReentrantLock;

public class SharedDataC extends SharedData {

    private final ReentrantLock _lock = new ReentrantLock();
    private final Condition _canRead = _lock.newCondition();
    private final Condition _canWrite = _lock.newCondition();
    private final AtomicInteger _count = new AtomicInteger(0);

    public SharedDataC(int x, int y) {
        super(x, y);
    }

    @Override
    public void move(int x, int y) {

        // Locking
        _lock.lock();

        try {
            // Waiting for signal that there are no readers
            if (!_count.compareAndSet(0, 0)) {
                _canWrite.await();
            }

            super.move(x, y);

            // Signals readers
            _canRead.signalAll();
        } catch (InterruptedException ex) {
            ex.printStackTrace();
        } finally {
            // Unlocking
            _lock.unlock();
        }
    }

    public SharedData get() {

        // Waiting while a writer wants to update
        while (_lock.isLocked()) {
            try {
                _canRead.await();
            } catch (InterruptedException ex) {
                ex.printStackTrace();
            }
        }

        // Atomically incrementing readers count
        _count.incrementAndGet();

        SharedData sd = super.get();

        int a = _count.decrementAndGet();
        if (a == 0 && _lock.isLocked()) {
            _canWrite.signalAll();
        }
        return sd;

    }

}

SharedDataC (Need to take care of the sync demands in this class) Added this alternative implementation after Thomas comment:

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantReadWriteLock;

public class SharedDataC extends SharedData {

    private final ReentrantReadWriteLock rwl = new ReentrantReadWriteLock();
    private final Lock r = rwl.readLock();
    private final Lock w = rwl.writeLock();

    public SharedDataC(int x, int y) {
        super(x, y);
    }

    @Override
    public void move(int x, int y) {
        w.lock();
        try { super.move(x, y); }
        finally { w.unlock(); }
    }

    @Override
    public SharedData get() {
        r.lock();
        try { return super.get(); }
        finally { r.unlock(); }
    }
}

Reader:

public class ReaderThread extends Thread {

    private final SharedData _shData;

    public ReaderThread(SharedData shData){
        super();
        _shData = shData;
    }

    @Override
    public void run() {
        for (int i = 0; i < 10; i++){
            SharedData sd = _shData.get();
            System.out.println("Consumer " + getName() + ": " + sd);
            try {
                sleep((long)(Math.random() * 2000));
            } catch (InterruptedException ex) {
                ex.printStackTrace();
            }
        }
    }
}

Writer:

public class WriterThread extends Thread {
    private SharedData _shData;


    public WriterThread(SharedData shData){
        super();
        _shData = shData;
    }

    @Override
    public void run() {
        for (int i=0; i < 10; i++){
            _shData.move((int)(Math.random()*100), (int)(Math.random()*100));
            System.out.println("Producer " + getName() + ": " + _shData);
            try {
                sleep((long)(Math.random() * 2000));
            } catch (InterruptedException ex) {
                ex.printStackTrace();
            }
        }
    }
}

Tester class:

    public class Tester {

        public static void main(String[] args){
        SharedDataC shDataC = new SharedDataC(0,0);
        WriterThread thpC1 = new WriterThread(shDataC);
        WriterThread thpC2 = new WriterThread(shDataC);
        WriterThread thpC3 = new WriterThread(shDataC);
        WriterThread thpC4 = new WriterThread(shDataC);
        ReaderThread thcC1 = new ReaderThread(shDataC);
        ReaderThread thcC2 = new ReaderThread(shDataC);
        ReaderThread thcC3 = new ReaderThread(shDataC);
        ReaderThread thcC4 = new ReaderThread(shDataC);
        thpC1.start();
        thpC2.start();
        thpC3.start();
        thpC4.start();
        thcC1.start();
        thcC2.start();
        thcC3.start();
        thcC4.start();
        }
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ Why implement that thing yourself and not use ReadWriteLock? \$\endgroup\$ – Thomas Weller Jun 2 at 22:21
  • 1
    \$\begingroup\$ The test does not check the result. Also, the sleeps are quite long for testing. To get data races, I'd test at full speed (100% CPU) with no sleeps and many more loop iterations. \$\endgroup\$ – Thomas Weller Jun 2 at 22:39
  • \$\begingroup\$ To me it seems that you could have reader starvation in your implementation, since the _lock has precedence. \$\endgroup\$ – Thomas Weller Jun 2 at 22:41
  • 1
    \$\begingroup\$ @ThomasWeller I added an alternative implementation with the class you suggested. Would still appreciate comments on both, for learning purposes \$\endgroup\$ – user97059 Jun 2 at 22:42
  • 1
    \$\begingroup\$ @ratchetfreak I have read this code before. I believe many parts are copied from ArrayBlockingQueue; \$\endgroup\$ – dfhwze Jun 3 at 19:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy