Unit testing for JBCACHE-315 has gotten my head a bit into locking issues and I ended up drifting into JBCACHE-285-land.

I think the fundamental problem with our READ_COMMITTED handling is that once a thread has a read lock, IdentityLock will allow it another read whether or not another thread has subsequently acquired a write lock. This can be solved by requiring a second acquisition of the read lock if the isolation level is READ_COMMITTED. For example, a re-implementation of IdentityLock.acquireReadLock():

public boolean acquireReadLock(Object caller, long timeout)
 throws LockingException, TimeoutException, InterruptedException
 {
 boolean rc;

 if (caller == null) {
 throw new IllegalArgumentException("owner is null");
 }

 boolean hasRead = false;
 boolean hasRequired = false;
 if (mustReacquireRead_)
 {
 hasRequired = map_.isOwner(caller, LockMap.OWNER_WRITE);
 if (!hasRequired)
 hasRead = map_.isOwner(caller, LockMap.OWNER_READ);
 }
 else if (map_.isOwner(caller, LockMap.OWNER_ANY)) {
 hasRequired = true;
 }

 if (hasRequired) {
 if (log.isTraceEnabled()) {
 StringBuffer sb=new StringBuffer(64);
 sb.append("acquireReadLock(): caller ").append(caller).append(" already owns lock for ").append(fqn);
 log.trace(sb.toString());
 }
 return false; // owner already has the lock
 }

 rc = lock_.readLock().attempt(timeout);

 // we don't need to synchronize from here on because we own the semaphore
 if (rc == false) {
 StringBuffer sb = new StringBuffer();
 sb.append("read lock for ").append(fqn).append(" could not be acquired by ").append(caller);
 sb.append(" after ").append(timeout).append(" ms. " + "Locks: ").append(map_.printInfo());
 sb.append(", lock info: ").append(toString(true));
 log.error(sb.toString());
 throw new TimeoutException(sb.toString());
 }

 // Only add to the map if we didn't already have the lock
 if (!hasRead)
 map_.addReader(caller); // this is synchronized internally, we don't need to synchronize here
 return true;
 }

Instance variable mustAcquireRead_ is set in the c'tor by checking if the cache's isolation level is READ_COMMITTED.

The "reacquire read lock" behavior is only needed for READ_COMMITTED. With stronger isolation, the original read lock will prevent a subsequent write lock, so no need to worry about a dirty read. With weaker isolation, we don't care about the dirty read.

There are a couple things about this solution I don't like too much:

1) IdentityLock is varying its behavior based on IsolationLevel. From a purity of design point of view, it's nicer to deal with this in the LockStrategy. But, OTOH, all the places where IdentityLock checks the LockMap and then makes decisions already represent a case of IdentityLock getting involved with locking strategy.

2) You end up with multiple calls to lock.readLock().attempt() by the same lock owner, but there will only be one release() call. This means NonBlockingWriterLock's activeReaders int field will be incorrect. However, AFAICT in NonBlockingWriterLock this field is not used in any decision making -- the number of activeReaders doesn't matter because a writer thread can acquire a lock no matter how many readers there are.

There are two failing unit tests related to READ_COMMITTED semantics, IsolationLevelReadCommittedTest and LockTest. The former test passes with this fix in place. The latter operates directly on the LockStrategy impl, not IdentityLock, so this fix doesn't resolve the test failure. But, changing the test to add a second read lock acquisition makes a passable test possible.

I haven't run the full test suite yet to see if this change breaks anything else.

Any thoughts?