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?

If `magic()` throws a RuntimeException in option 2, then foo won't be unlocked. — Cory Kendall, May 05 '13 at 07:08
@Cory that's a mistake on my transcription. Option 2 is supposed to have a second finally block. — corsiKa, May 05 '13 at 07:44

score 7 · Accepted Answer · answered May 05 '13 at 07:07

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();
    }

That's an excellent example. In this case, they need to plug up both ends of the queue to prevent someone from sneaking an extra element in. — corsiKa, May 06 '13 at 14:38

score 5 · Answer 2 · answered May 05 '13 at 07:09

5

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.

answered May 05 '13 at 07:09

JB Nizet

678,734
91
1,224
1,255

That was a transcription error. Whoops. And I've already documented the order in which multiple locks need to be acquired. – corsiKa May 05 '13 at 07:46

Evgeniy Dorofeev · Answer 3 · 2013-05-05T07:43:50.430

0

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();
    }

edited May 05 '13 at 07:43

answered May 05 '13 at 07:18

Evgeniy Dorofeev

133,369
30
199
275

In the javadoc I read, lock() doesn't throw any exception, and unlock() only throws an exception is you're not the owner of the lock, which is not possible in the given code. – JB Nizet May 05 '13 at 07:26
I totally agree. No such exception in the APIs and looking through Doug Lea's source on the locks. Even write locks don't throw. – Jeremy Unruh May 05 '13 at 07:30
But this is what lock() API says: Lock implementation may be able to detect erroneous use of the lock, such as an invocation that would cause deadlock, and may throw an (unchecked) exception in such circumstances. – Evgeniy Dorofeev May 05 '13 at 07:32
Besides, this Lock usage example is from API too: l.lock(); try { // access the resource protected by this lock } finally { l.unlock(); } – Evgeniy Dorofeev May 05 '13 at 07:34
2

Then we're not reading the same API doc. Here's mine: http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.WriteLock.html#lock%28%29. Anyway, if the programmer uses the lock incorrectly, it's a bug and unlocking it correctly won't make the bug disappear and the program work correctly. I agree that your code is correct. But option 1 is also. – JB Nizet May 05 '13 at 07:36
I have to admit, I'm not concerned if it throws an exception due to deadlock. Either way, the system is in an inconsistent state - there is little you can do to remedy such a state at runtime anyway, so an exception isn't very useful. – corsiKa May 06 '13 at 14:40

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

Option 1

Option 2

3 Answers3

Linked