RAII pattern for downgradable ReadWriteLock

The example for downgrading ReentrantReadWriteLock in the Java documentation seems really unsafe when handling exceptions. I tried to write a simple class to simplify it. Do you see any cases where it can fail?

I’m looking for a review from a concurrency point of view, such as if this code may fail if an exception is thrown inside a nested lock before the downgrade.

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

public class RWLockWrapper implements AutoCloseable {
    private final ReadWriteLock rwl;
    private Lock current;

    //RAII
    public RWLockWrapper(ReadWriteLock rwl) {
        this.rwl = rwl;
        this.current = rwl.writeLock();
        this.current.lock();
    }

    public void downgrade() {
        final Lock read = rwl.readLock();
        read.lock();
        try {
            current.unlock();
        } finally {
            current = read;
        }
    }

    @Override
    public void close() {
        current.unlock();
    }
}

To be used like this:

try (RWLockWrapper lock = new RWLockWrapper(rwl)) {
    //write locked

    lock.downgrade();

    //read locked
}

Your concurrency/integrity of the code, and the way you have shown it used, are fine.

The design, on the other hand, is very questionable…. I would recommend that you do not use this pattern at all.

If you are going to use it (your comment here says it is used already), then I recommend changing the name. It is not a wrapper, since you can’t actually access the lock through the wrapper. Instead, it is a ‘Locked Session’, which can be closed, and downgraded. The following code looks/reads better to me:

try (WriteLockedSession sessionlock = new WriteLockedSession(rwl)) {
    //write locked

    writelock.downgrade();

    //read locked
}

This makes it clear that the constructor starts a Write-lock, and the session is ‘closable’. It can also be downgraded.

Concurrency in any language is complicated. Java has four well-documented mechanisms:

1. single-thread only
2. volatile
3. synchronized
4. java.util.concurrent.*
   • ‘self-contained’ classes and structures
   • atomic.* micro-locking systems
   • locks.* macro-locking systems

Correct use of each of these ‘systems’ will allow safe operation.

Problems start to happen when things get non-standard, and flow against the standard usage patterns. When people cannot ‘see’ the concurrency-controls, that’s when bugs happen.

Your class imposes coding practices that are unexpected:

• it ‘hides’ the locking
• it requires the use of the try-with-resources pattern instead of try-finally
• it assumes that the implementation of the ReadWriteLock is compatible with your algorithm.

The last point is significant…. The ReadWriteLock does not specify the implementation details of the lock, it is simply an interface. Additionally, it explicitly says that implementations can impose restrictions:

Although the basic operation of a read-write lock is straight-forward, there are many policy decisions that an implementation must make…:

• Can it acquire a read lock while holding the write lock?
• Can the write lock be downgraded to a read lock without allowing an intervening writer?
• ….

Your code assumes the above will just work, but that is a bad assumption.

I recommend you change your code to work on the actual ReentrantReadWriteLock implementation provided in the concurrent package. It does support these things.

Also, the following code in your downgrade method is unnecessary:

    try {
        current.unlock();
    } finally {
        current = read;
    }

This may as well just be:

        current.unlock();
        current = read;

unlock() can’t throw an exception. If it did, everything else in Java would be broken too. the try/finally block suggests you are protecting the lock somehow, but it is just fluff.

Having pointed out those issues, consider the alternative….

Now, what exactly is wrong with a standard pattern?

Consider this:

ReentrantReadWriteLock rwl = .....

Lock lock = rwl.writeLock();
lock.lock();
try {

    rwl.readLock().lock();
    lock.unlock();
    lock = rwl.readLock()

} finally {
    lock.unLock();
}

The above has the try-finally block that is common, it does not hide anything, so the concurrency-controls are all visible in a single place, and it is really not much longer than your existing code.

You didn’t give a terrible amount of code to be reviewed, and I am not really proficient in concurrency and the like but I still got a few points I want to make.

But enough of introduction let’s jump into the code:

public class RWLockWrapper implements AutoCloseable {

Wonderful, concise and clear. RWLockWrapper is a beatiful name for that class. You could have spelled it out, but that is definitely not much improvement in clarity.

I also like the design choice of implementing AutoCloseable. This makes your wrapper easy and obvious to use.

One big thing that’s missing here: Documentation! I strongly suggest you write a little JavaDoc for this. It doesn’t even need to be much, it should just explain the class in short and inform the user about the caveats of using it.

private final ReadWriteLock rwl;
private Lock current;

oh sweet joy! You made your ReadWriteLock read-only. I like that design choice for two reasons: You make clear that the Wrapper is usable for only one and exactly one Lock at a time, and additionally you cleanly make it impossible to do any different.

The name could be better, but it’s a short class, so it’s definitely not very detrimental to have a name like rwl.

As an aside: current could probably use a better name, but for the love of {higher entity of choice} I can’t find a better one…

//RAII
public RWLockWrapper(ReadWriteLock rwl) {
    this.rwl = rwl;
    this.current = rwl.writeLock();
    this.current.lock();
}

The first thing that I really find not acceptable in your code is that comment.

It’s utterly useless, and for me (who doesn’t use locks all that much) uninformative and confusing. Again I strongly recommend you document what you are doing here.

As a no-experience user, I’d have some expectations to your class, and that you write-lock things, is definitely not the first thing I’d think 🙁

JavaDoc can really help a lot.

Additional minor nitpicks: I personally put a space between the name and the opening parens, I feel I can read that better.

public RWLockWrapper (ReadWriteLock rwl) {

Furthermore you are overqualifying current with this., which I don’t like. I personally prefer to not use this wherever possible, but you may decide to do differntly so.

public void downgrade() {
    final Lock read = rwl.readLock();
    read.lock();
    try {
       current.unlock();
    } finally {
       current = lock;
    }
}

not much to say here. Again I’d prefer a little more whitespace, again missing documentation. Also you may want to mention in possible documentation, that the state of the Lock will always be locked read after calling this method.

Moving swiftly on. Where’s the upgrade ()????

Confusion ensues

What, there is no upgrade?? I think you get it.
When you expose some downgrade (), I expect to also be able to upgrade ().

I don’t really understand why you don’t expose that method. I suspect you have no real need for it. I feel that upgrade/downgrade is a pair just like add/remove, create/destroy and similar.

That aside, moving swiftly on.

@Override
public void close() {
    current.unlock();
}

Hmmm…. Okay for the sake of the argument we assume, that calling unlock() will not throw any exceptions…

No. There is always the possibility for an execption to be thrown, even more so, when you don’t really secure yourself against it…

But that aside, hypothetical question: What would happen if someone did the following

    RWLockWrapper myCoolLock = new RWLockWrapper(lockForTotallyImportantAndCriticalThing);
    //... do some important stuff.
}

Oh crap. Oh crap… Everything broke!!

I think you see what I am getting at. Given the case someone uses your wrapper wrongly, things can go wrong easily.

I suggest you consider adding:

@Override
public void finalize () {
    current.unlock();
}

Why that? To make sure, that your Lock does not stay locked when you can never unlock it again, because you lost it in a tangled mess of bits and bytes.

This should provide additional security to the wrapper even against non-proficient, blue-eyed or just plain {insert favourite insult, doubting intelligence} programmers.