Java Sonar/FindBugs - Multithreaded correctness

Question

Sonar reports following issue:

Multithreaded correctness - Method does not release lock on all
exception paths findbugs : UL_UNRELEASED_LOCK_EXCEPTION_PATH
This method acquires a JSR-166 (java.util.concurrent) lock, but does not release it on all exception paths out of the method. In general, the correct idiom for using a JSR-166 lock is:  



Lock l = ...;
l.lock();
try {
    // do something
} finally {
    l.unlock();
}

On this code snippet:

   public synchronized void put(T element) throws PreconditionException {
        Precondition.notNull(element, "Nullobject is not allowed");
        lock.lock();
        try {
            buffer[writeIndex++] = element;
            if (writeIndex >= capacity) {
                writeIndex = 0;
            }
            if (size.get() >= capacity) {
                // buffer is full
                readIndex++;
                lastOverflow.set(System.currentTimeMillis());
                if (readIndex >= capacity) {
                    readIndex = 0;
                }
                return;
            }
            size.incrementAndGet();
        } finally {
            try {
                notEmpty.signal();
            } catch (Exception e) {
               e.printStackTrace();
            }
            lock.unlock();
        }
    }

I won't get it, for what reason, this is not safe?

Answer 1

I think it shows that because if your code throws another subclass of Throwable (such as Error) on the line notEmpty.signal(); then the lock won't be released.

You can argue that if there's an Error, your application will shutdown or that catching Exception is the right thing to do in this code. I do agree on that, but as FindBugs says, there are some code paths that end up in that lock not being released... and nothing stops a developer from extending Throwable :'(.

Answer 2

You can add a finally for the unlock to make sure it is called, in contrast to a possible exception path that could lead out of your try-catch.

Besides, catching an exception and just logging it, should not be kept for production, as you surely are aware.

} finally {
            try {
                notEmpty.signal();
            } catch (Exception e) {
               e.printStackTrace();
            }
            finally {
               lock.unlock();
            }
        }