Checking for readLock possession in `ReentrantReadWriteLock`

Question

I have a few closely related questions that popped up while trying to use ReentrantReadWriteLock to control access to a fairly complex data structure that has a number of different read and write operations. As per examples in the documentation, I've fetched a read and a write lock, and I'm using them to (from what I can tell) successfully manage concurrent access to this data structure. However, while debugging another issue, I noticed that sometimes I acquire more than one read lock in the same thread. The basic reason for this is that I have a number of complex queries that invoke simpler ones (see the example below). The complex query can be thought of as a transaction, i.e., there should be no writes between the getVersion() and the access to data in myQuery. This current solution works, but it means that in some places in the code, I will have multiple read locks possessed by the same thread. I've noticed that the write equivalent has an isHeldByCurrentThread() method, but this is oddly absent in readLock. I couldn't find out the following:

• is it bad (poor style, performance or risk of future bugs) to have multiple read locks in the same thread?
• is is bad to use WriteLock.isHeldByCurrentThread to check for errors (e.g., expecting a write lock but none is present, or as a condition to release the write lock)?
• is there a best practice to decide how to proceed if this is an issue? Do I pass a lockedAquired boolean to methods that may be called from code that already possesses a lock, do I ensure they are uniquely acquired, or should I use ReadLock.tryLock()?

Here's the code sample:

public class GraphWorldModel {

  //unfair RW Lock (default, see javadoc)
  protected final ReentrantReadWriteLock rwl = new ReentrantReadWriteLock(false);
  protected final ReentrantReadWriteLock.ReadLock readLock = rwl.readLock();
  protected final ReentrantReadWriteLock.WriteLock writeLock = rwl.writeLock();

  protected long versionID = 0;
  protected Object data = null;

  @Override
  public long getVersion() {
      long result = -1;
      readLock.lock();
      try {
          result = this.versionID;
      } finally {

          readLock.unlock();
      }
      return result;
  }
  public ResultObject myQuery() {
      //do some work
      readLock.lock();
      try {
        long version = getVersion();
        ResultObject result = new ResultObject(this.data, version);
        //note: querying version and result should be atomic!
      } finally {
        readLock.unlock();
      }
      //do some more work
      return result;
  }
}

Answer 1

• is it bad (poor style, performance or risk of future bugs) to have multiple read locks in the same thread?

I would say it is questionable style. Firstly, as @JBNizet points out, there are no problems with you trying to acquire a lock you already own again. The lock just increments the reader count by one and then decrements it in the finally unlock.

However, it does mean that you have to cross a memory barrier (volatile read) to update the shared lock statistics which means a performance hit. It depends on how frequent this code is executed as to whether or not it is going to make any noticeable performance difference.

In terms of bugs however, there is a big gotcha. If you are talking about just read-only locks then I don't think there is more chance of bugs. Actually trying to fix the double lock problem (see below) probably has more risk. But in your code, the try/finally logic ensures that the locks are used appropriately and unlocked when done.

However, if you are talking about mixing read and write locks then you need to understand that the following code deadlocks:

ReentrantReadWriteLock rrwl = new ReentrantReadWriteLock();
ReadLock readLock = rrwl.readLock();
WriteLock writeLock = rrwl.writeLock();
readLock.lock();
writeLock.lock();

Or at least it stops until some other code unlocks the read-lock. If you are going to mix read and write locks in your code then you must protect against double locking.

• is is bad to use WriteLock.isHeldByCurrentThread to check for errors (e.g., expecting a write lock but none is present, or as a condition to release the write lock)?

This is going to add a lot of logic to your code which is more risky but maybe worth it if the performance hit is large. That said, see below for an alternative.

• is there a best practice to decide how to proceed if this is an issue? Do I pass a lockedAquired boolean to methods that may be called from code that already possesses a lock, do I ensure they are uniquely acquired, or should I use ReadLock.tryLock()?

What I would do is to create a kernel method called getVersionLocked(). Something like:

@Override
public long getVersion() {
    readLock.lock();
    try {
        // NOTE: I refactored this to return out of the try/finally which is a fine pattern
        return getVersionLocked();
    } finally {
        readLock.unlock();
    }
}
public ResultObject myQuery() {
    //do some work
    readLock.lock();
    ResultObject result;
    try {
      long version = getVersionLocked();
      result = new ResultObject(this.data, version);
      //note: querying version and result should be atomic!
    } finally {
      readLock.unlock();
    }
    //do some more work
    return result;
}
// NOTE: to call this method, we must already be locked
private long getVersionLocked() {
    return this.versionID;
}

Then you can make the decision to call the locked or unlocked version of your method and only do the readLock() once.

This is more risky in that you might call getVersionLocked() when the lock is not held but I think it is an acceptable risk.