Fair ReadWriteLock with Conditions

Question

First of all I checked previous questions about this topic, however none fits my specific problem.

I got the following Code that illustrates sensors with a timestamp and data stored in a double array, additionally an instance of my implemented FairRWLock.

class LockedSensors implements Sensors {
    long time = 0;
    double data[];
    FairRWLock lock = new FairRWLock();

    LockedSensors() {
        time = 0;
    }

    // store data and timestamp
    // if and only if data stored previously is older (lower timestamp)
    public void update(long timestamp, double[] data) {
        lock.writeAcquire();
            if (timestamp > time) {
                if (this.data == null)
                    this.data = new double[data.length];
                time = timestamp;
                for (int i = 0; i < data.length; ++i)
                    this.data[i] = data[i];
            }
        lock.writeRelease();
    }

    // pre: val != null
    // pre: val.length matches length of data written via update
    // if no data has been written previously, return 0
    // otherwise return current timestamp and fill current data to array passed
    // as val
    public long get(double val[]) {
        try{
            lock.readAcquire();
                if (time == 0) return 0;
                for (int i = 0; i < data.length; ++i)
                    val[i] = data[i];
                return time;
        } finally{lock.readRelease();}
    }
}

It supports an update, which depends on the time when new data was received, and get, which extracts the data stored in the specific sensor.

This is my implementation of the FairRWLock:

class FairRWLock{
    private int readers = 0, writers = 0, readersWaiting = 0, writersWaiting = 0, writersWait = 0;
    private static final int ReaderPriority = 30;
    private Lock lock = new ReentrantLock();
    private Condition readerPass = lock.newCondition();
    private Condition writerPass = lock.newCondition();
    /*
     * readers denotes the number of current readers, writers equivalent, readersWaiting denotes the number of readers
     * awaiting their signal, writersWaiting equivalent. writersWait denotes the number of readers the writers have to
     * let pass before they can proceed, this amount is controlled by the ReaderPriority (reset occurs when writer releases)
     */


    /*
     * increment the number of waiting readers, check if there are any currently working writers OR writers are waiting
     * whilst they don't have to let any readers pass. When signaled, decrement readersWaiting, decrement the number of
     * readers the writers have to let pass and increment the number of current readers.
     */
    public void readAcquire(){
        lock.lock();
            readersWaiting++;
            while(writers > 0 || (writersWaiting > 0 && writersWait <= 0)){
                try {
                    readerPass.await();
                } catch (InterruptedException e) {}
            }
            readersWaiting--;
            writersWait--;
            readers++;
        lock.unlock();
    }

    /*
     * simply decrement number of readers and signal the threads that have to be signaled
     */
    public void readRelease(){
        lock.lock();
            readers--;
            signaling();
        lock.unlock();
    }

    /*
     * increment number of waiting writers, check if there are currently working writers OR readers OR readers currently
     * have priority over the writers. When signaled decrement writersWaiting, increment number of writers
     */
    public void writeAcquire(){
        lock.lock();
            writersWaiting++;
            while(writers > 0 || readers > 0 || (readersWaiting > 0 && writersWait > 0)){
                try{
                    writerPass.await();
                } catch(InterruptedException e) {}
            }
            writersWaiting--;
            writers++;
        lock.unlock();
    }

    /*
     * simply decrement number of current writers, reset the number of readers the writers have to let pass before
     * another writer may pass. signal the ones that should be
     */
    public void writeRelease(){
        lock.lock();
            writers--;
            writersWait = ReaderPriority;
            signaling();
        lock.unlock();
    }

    /*
     * check first if readers currently got priority over the writers. if so (readersWaiting??) ? signal them : signalAll,
     * if not (writersWaiting??) ? signal them : signalAll
     */
    private void signaling(){
        if(writersWait > 0){
            if(readersWaiting > 0) readerPass.signalAll();
            else writerPass.signal();
        } else{
            if(writersWaiting > 0) writerPass.signal();
            else readerPass.signalAll();
        }
    }
}

I'm not very familiar with the locking by conditions and it seems my code suffers either from starvation or even deadlock. However I can't find the issue (which most probably is somewhere in the FairRWLock implementation).

Is there a reason you didn't use `ReentrantLock` with the optional fairness parameter instead of writing a `FairRWLock` yourself? — Kayaman, Jul 11 '17 at 11:39
Just tested the code with the provided fair ReentrantReadWriteLock and also with a similar implementation to the one above, but with monitoring (wait and notifyAll calls). The monitoring one proceeds much faster than the one with the provided fair lock, so it should be possible to improve even further with the idea above. — Greg, Jul 11 '17 at 22:01
This doesn’t make sense. The intrinsic monitor is not fair, so the fact that it is faster than a fair lock is a) not surprising and b) no prove that you can implement a faster fair lock. But fair locks are an xy problem in general. Concurrent code should not need fair locks in the first place. — Holger, Jul 12 '17 at 07:42

score 2 · Accepted Answer · answered Jul 12 '17 at 08:23

2

There is no sense in trying to build a fair lock atop an unfair lock. Right when threads enter either readAcquire() or writeAcquire(), they are calling lock.lock() and if that doesn’t succeed immediately, they may be put into the wait state and get overtaken by an arbitrary number of threads before they can proceed.

At this point, it is already impossible to reestablish fairness by whatever you do afterwards. But it’s worth noting that you are also missing the implications of await(). This operation will release the lock temporarily, as only that gives other threads a chance to fulfill the condition you are awaiting. When the thread gets signal()ed, it has to re-acquire the lock, which is again, not a fair operation. An arbitrary number of threads may make a new lock request, changing the situation entirely before the thread having called await() long ago will proceed.

In the end, you do not want fairness. The update operation is intended to ignore outdated updates, so it would actually be a win if newer update request can proceed faster, as the pending older ones will become no-ops then. For concurrent get requests, you actually don’t want blocking at all, all read requests should be able to run concurrently, but, of course, you want consistency (thread safety) and no writer starvation here.

The best solution is to do no locking at all and implement the entire operation lock-free:

class LockedSensors implements Sensors {
    private static final class State {
        final long time;
        final double[] data;
        State(long t, double[] in) {
            time = t;
            data = in.clone();
        }
    }
    final AtomicReference<State> current = new AtomicReference<>();
    LockedSensors() {}

    // store data and timestamp
    // if and only if data stored previously is older (lower timestamp)
    public void update(long timestamp, double[] data) {
        State newState = null;
        for(;;) {
            State old = current.get();
            if(old != null && old.time > timestamp) return;
            if(newState == null) newState = new State(timestamp, data);
            if(current.compareAndSet(old, newState)) return;
        }
    }

    // pre: val != null
    // pre: val.length matches length of data written via update
    // if no data has been written previously, return 0
    // otherwise return current timestamp and fill current data to array passed as val
    public long get(double[] val) {
        State actual = current.get();
        if(actual == null) return 0;
        if(actual.data.length != val.length)
            throw new IllegalArgumentException();
        System.arraycopy(actual.data, 0, val, 0, actual.data.length);
        return actual.time;
    }
}

Here, readers can always proceed returning the result of the last completed update while never blocking any writers. Even writers do not block each other, but may have to spin if another update happened in-between, but since every writer will return as soon as a newer timestamp has been encountered and there will always be at least one writer making progress, there is no problem here.

answered Jul 12 '17 at 08:23

Holger

285,553
42
434
765

I did not read entirely but those names look skewed, a read is an `acquire` indeed, a *write* is a release; it looks like that should be `writeRelease`. that was a fast accept... :) +1 i guess, but still reading – Eugene Jul 12 '17 at 09:05
@Eugene: no, they are actually the equivalent of a `readWriteLock.readLock().lock()` and `readWriteLock.writeLock().lock()`. – Holger Jul 12 '17 at 09:09
@Holger: In the meanwhile I made the code from above work, as you explained though the forced "fair, actually not fair" locking lead to a massive overhead, which I now can fully understand because of your answer. However the whole thing was for my own practice purpose, I came already up with a lock-free (in this case even wait-free) implementation with a do{ }while(!compareAndSet) in the update instead. Thanks for the accurate explanation. – Greg Jul 12 '17 at 09:22
@Holger that's interesting... don't you want to do the entire update method under a spin lock? Are operations guaranteed not to overflow past `for(;;)`? Also looks like a good opportunity for jdk-9 `Thread.onSpinWait` – Eugene Jul 12 '17 at 09:28
@Eugene: I have no idea what you mean with “overflow past `for(;;)`”. The whole point of this implementation is not to need a lock. – Holger Jul 12 '17 at 11:43
@Holger well yes, but you still have a spin-lock here via `for(;;)` and `compareAndSet`. By overflow I meant re-ordering – Eugene Jul 12 '17 at 11:45
@Eugene: there is a single cas in a loop, just like in a spin lock, but there is no release action required, hence, it is not a lock. Since cas is a thread safe operation including the required memory model semantics, there are no re-ordering issues possible. Note that the `State` class is immutable and would even work with an unsafe publication through the additional `final` field publication guarantees, though they are not needed here as `compareAndSet` is sufficient. – Holger Jul 12 '17 at 11:53
@Holger wait... that `compareAndSet` both reads (acquire) and writes (release) a volatile field, in my mind that *is* acquire/release semantics. The re-orderings statement (happens-before) is indeed invalid, my bad. As far as I can tell a simple volatile write would be enough here. – Eugene Jul 12 '17 at 12:06
@Holger and let me put it in a different angle: there is absolutely no one @ my workplace that I could debate this with nor have a I written code like in a production env - thus please don't be harsh with me if I make a statement that contradicts with your vast (assuming) experience here. – Eugene Jul 12 '17 at 12:11
@Eugene: you are right, a simple `volatile` write would be sufficient for a safe publication, cas is only needed to ensure that we don’t overwrite a newer value in case of concurrent updates. The additional `volatile` read semantics has no performance impact, however, as there’s a `volatile` read right before it anyway, so no operation before or after the loop can get reordered in respect to the loop and the loop’s body is tiny. It’s best to see the entire `update` method as a single atomic operation when looking at the interaction with the rest of the program. – Holger Jul 12 '17 at 12:15
@Holger yes that was sort of my point, `update` should be viewed as atomic and as such I would expect it's entire body to be spin lock protected... It's much simpler to reason in this case - at least for me. – Eugene Jul 12 '17 at 12:30
@Eugene: but this is the standard idiom for implementing an atomic update, see the example in the [package description](https://docs.oracle.com/javase/8/docs/api/?java/util/concurrent/atomic/package-summary.html), adding another locking feature would complicate the code with no benefit. – Holger Jul 12 '17 at 12:33
@Holger I've been looking at this today also - and I do agree with u. that is perfectly valid implementation IMO. thank you – Eugene Jul 13 '17 at 10:23

Fair ReadWriteLock with Conditions

1 Answers1