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
}

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.

StackExchange Code Review Q#61702, answer score: 3