Is this lock management code snippet unnecessarily complicated?

Question

The Hewlett Packard Enterprise application Fortify On Demand contains this code snippet titled "Example 2: The following code will always release the lock."

ReentrantLock  myLock = new ReentrantLock();

try {
    myLock.lock();
    performOperationInCriticalSection();
    myLock.unlock();
}
finally {
    if (myLock != null) {
        myLock.unlock();
    }
}

Why there is the last line in the try block to unlock the lock, when it is already handled in finally? Is it really necessary, or is it just part of some verbose coding standard? The if looks unnecessary too.

Answer 1

That code throws an IllegalMonitorStateException, because it calls unlock() twice in succession.

From ReentrantLock.unlock:

Throws:

IllegalMonitorStateException - if the current thread does not hold this lock

I would have to say that the code is incorrect, and I can't really guess at what they were trying to do. It could work if there was a myLock = null; immediately after the unlock() inside the try block, but doing that is still sloppy code. If unlock() throws an exception, then the second invocation of unlock() will throw another exception too.

The correct idiom is like this, according to the documentation for e.g. Lock:

Lock l = ...;
l.lock();
try {
    // access the resource protected by this lock
} finally {
    l.unlock();
}

If you want to be really safe, you could use try-with-resources to take advantage of the fact that if close() throws an exception while an exception from the try block is being thrown, the close() exception is added to the suppressed exceptions so neither of them is lost. (In both the preceding example and the HP example, if unlock() throws an exception, the exception being thrown from the try block is entirely lost.)

class Locker implements AutoCloseable {
    private final Lock lock;
    Locker(Lock lock) { this.lock = Objects.requireNonNull(lock);
                        this.lock.lock(); }
    @Override public void close() { lock.unlock(); }
}

Lock lock = ...;
try (Locker locker = new Locker(lock)) {
    // If both modifySharedState() and unlock()
    // throw an exception, the unlock() exception
    // is added to the modifySharedState()
    // exception's suppressed list.
    modifySharedState();
}

(Here's an example output using that code on Ideone.)

Or you could write something like the code which try-with-resources translates to:

Lock lock = ...;
lock.lock();
Throwable primary = null;
try {
    modifySharedState();
} catch (Throwable x) {
    primary = x;
    throw x;
} finally {
    try {
        lock.unlock();
    } catch (Throwable x) {
        if (primary != null) {
            primary.addSuppressed(x);
            // primary is already "in-flight"
            // so no need to throw it again
        } else {
            throw x;
        }
    }
}

That's a bit hard to follow, though.

Accounting for unlock() throwing an exception could make sense if the logic is more complicated than just lock()...unlock(), for example if you had more than one lock or the unlock() was conditional for some reason.