Is it safe to lock multiple ReentrantReadWriteLocks in the same try block?

Question

Let's say I have two critial resources, foo and bar. I protect them with some ReentrantReadWriteLocks

ReentrantReadWriteLock foo = new RRWL() ...
ReentrantReadWriteLock bar = new RRWL() ...

Most operations only use foo OR bar, but some of them happen to use both. Now when using a single lock, you can't just do this:

void foo() {
   foo.writeLock().lock();
   privateWorkOnFoo();
   foo.writeLock().unlock();
}

If an exception is thrown, your foo will become forever locked. Instead you wrap it, like

void foo() {
    try {
        foo.writeLock().lock();
        privateWorkOnFoo();
    } finally { foo.writeLock().unlock(); }
}

But what if I need to work on both? Is it safe to put them in one block?

Option 1

try {
    foo.writeLock().lock();
    bar.writeLock().lock();
    magic();
} finally { 
    bar.writeLock().unlock();
    foo.writeLock().unlock();
}

Or is it necessary to give each lock its own block:

Option 2

try {
    foo.writeLock().lock();
    try {
        bar.writeLock().lock();
        magic();
    } finally { 
      bar.writeLock().unlock();
    }
    
} finally { 
    foo.writeLock().unlock();
}

I can't have been the first person to have hard to investigate this before... I know option 2 there is "bulletproof" but it's also a significant amount more maintenance. Is option 1 acceptable?

Answer 1

Option 1 is fine. It's known as the two lock variant. If you look at LinkedBlockingQueue operations such as remove, it locks the putLock as well as the takeLock. Here's a sample of what the JDK does:

  public boolean remove(Object o) {
       if (o == null) return false;
       fullyLock();
       try
       {
       // ...
       }   
       finally {
         fullyUnlock();
       }
    }

   /**
     * Lock to prevent both puts and takes.
     */
    void fullyLock() {
        putLock.lock();
        takeLock.lock();
    }

    /**
     * Unlock to allow both puts and takes.
     */
    void fullyUnlock() {
        takeLock.unlock();
        putLock.unlock();
    }

Answer 2

Option 1 is actually safer than Option 2, since if an exception is thrown in option 2, the second lock (foo) won't be unlocked: the unlocking is not in a finally block.

Also, be very careful when manipulating two locks, because there's a good chance of deadlock if one thread locks foo then bar, and another thread locks bar then foo.

Answer 3

According to Lock API both lock() and unlock() methods may throw an exception. So version 1 is incorrect because the second unlock may never be called. Version 2 is also incorrect, you should not call lock() inside try block because if lock.lock() threw an exception then it was not locked and you should not try to unlock it.

The correct version should be like this

    foo.writeLock().lock();
    try {
        bar.writeLock().lock();
        try {
            magic();
        } finally {
            bar.writeLock().unlock();
        }
    } finally { 
        foo.writeLock().unlock();
    }