Bug ID: JDK-6822903 Reliability and documentation improvements for ReentrantReadWriteLock

Type: Bug
Component: core-libs
Sub-Component: java.util.concurrent
Affected Version: 7

Priority: P4
Status: Resolved
Resolution: Fixed
OS: generic
CPU: generic

Submitted: 2009-03-26
Updated: 2016-02-10
Resolved: 2009-04-11

JDK 6	JDK 7
6u115Fixed	7 b55Fixed

Martin Buccholz writes:

The field firstReader is a long, and hence subject to word-tearing.
We fix that bug (which is very unlikely to cause a problem
in practice) and some documentation and clarity improvements.

SUGGESTED FIX # HG changeset patch > # User martin > # Date 1237237842 25200 > # Node ID 9c105cd4540322c7059d2044553069f1ef858a87 > # Parent 5854957b8b63fa364f686d3679be29c070139655 > [mq]: RRWL > > diff --git a/src/share/classes/java/util/concurrent/locks/ReentrantReadWriteLock.java > b/src/share/classes/java/util/concurrent/locks/ReentrantReadWriteLock.java > --- a/src/share/classes/java/util/concurrent/locks/ReentrantReadWriteLock.java > +++ b/src/share/classes/java/util/concurrent/locks/ReentrantReadWriteLock.java > @@ -276,7 +276,7 @@ > * Maintained as a ThreadLocal; cached in cachedHoldCounter > */ > static final class HoldCounter { > - int count; > + int count = 0; > // Use id, not reference, to avoid garbage retention > final long tid = Thread.currentThread().getId(); > } > @@ -293,8 +293,9 @@ > } > > /** > - * The number of read locks held by current thread. > + * The number of reentrant read locks held by current thread. > * Initialized only in constructor and readObject. > + * Removed whenever a thread's read hold count drops to 0. > */ > private transient ThreadLocalHoldCounter readHolds; > > @@ -304,17 +305,35 @@ > * where the next thread to release is the last one to > * acquire. This is non-volatile since it is just used > * as a heuristic, and would be great for threads to cache. > + * > + * Can outlive the Thread for which it is caching the read > + * hold count, but avoids garbage retention by not retaining a > + * reference to the Thread. > + * > + * Accessed via a benign data race; relies on the memory > + * model's final field and out-of-thin-air guarantees. > */ > private transient HoldCounter cachedHoldCounter; > > /** > * firstReader is the first thread to have acquired the read lock. > * firstReaderHoldCount is firstReader's hold count. > - * This allows tracking of read holds for uncontended read > + * > + * More precisely, firstReader is the unique thread that last > + * changed the shared count from 0 to 1, and has not released the > + * read lock since then; null if there is no such thread. > + * > + * Cannot cause garbage retention unless the thread terminated > + * without relinquishing its read locks, since tryReleaseShared > + * sets it to null. > + * > + * Accessed via a benign data race; relies on the memory > + * model's out-of-thin-air guarantees for references. > + * > + * This allows tracking of read holds for uncontended read > * locks to be very cheap. > */ > - private final static long INVALID_THREAD_ID = -1; > - private transient long firstReader = INVALID_THREAD_ID; > + private transient Thread firstReader = null; > private transient int firstReaderHoldCount; > > Sync() { > @@ -393,16 +412,16 @@ > } > > protected final boolean tryReleaseShared(int unused) { > - long tid = Thread.currentThread().getId(); > - if (firstReader == tid) { > + Thread current = Thread.currentThread(); > + if (firstReader == current) { > // assert firstReaderHoldCount > 0; > if (firstReaderHoldCount == 1) > - firstReader = INVALID_THREAD_ID; > + firstReader = null; > else > firstReaderHoldCount--; > } else { > HoldCounter rh = cachedHoldCounter; > - if (rh == null || rh.tid != tid) > + if (rh == null || rh.tid != current.getId()) > rh = readHolds.get(); > int count = rh.count; > if (count <= 1) { > @@ -416,6 +435,9 @@ > int c = getState(); > int nextc = c - SHARED_UNIT; > if (compareAndSetState(c, nextc)) > + // Releasing the read lock has no effect on readers, > + // but it may allow waiting writers to proceed if > + // both read and write locks are now free. > return nextc == 0; > } > } > @@ -450,15 +472,14 @@ > if (!readerShouldBlock() && > r < MAX_COUNT && > compareAndSetState(c, c + SHARED_UNIT)) { > - long tid = current.getId(); > if (r == 0) { > - firstReader = tid; > + firstReader = current; > firstReaderHoldCount = 1; > - } else if (firstReader == tid) { > + } else if (firstReader == current) { > firstReaderHoldCount++; > } else { > HoldCounter rh = cachedHoldCounter; > - if (rh == null || rh.tid != tid) > + if (rh == null || rh.tid != current.getId()) > cachedHoldCounter = rh = readHolds.get(); > else if (rh.count == 0) > readHolds.set(rh); > @@ -485,19 +506,17 @@ > int c = getState(); > if (exclusiveCount(c) != 0) { > if (getExclusiveOwnerThread() != current) > - //if (removeNeeded) readHolds.remove(); > return -1; > // else we hold the exclusive lock; blocking here > // would cause deadlock. > } else if (readerShouldBlock()) { > // Make sure we're not acquiring read lock reentrantly > - long tid = current.getId(); > - if (firstReader == tid) { > + if (firstReader == current) { > // assert firstReaderHoldCount > 0; > } else { > if (rh == null) { > rh = cachedHoldCounter; > - if (rh == null || rh.tid != tid) { > + if (rh == null || rh.tid != current.getId()) { > rh = readHolds.get(); > if (rh.count == 0) > readHolds.remove(); > @@ -510,25 +529,20 @@ > if (sharedCount(c) == MAX_COUNT) > throw new Error("Maximum lock count exceeded"); > if (compareAndSetState(c, c + SHARED_UNIT)) { > - long tid = current.getId(); > if (sharedCount(c) == 0) { > - firstReader = tid; > + firstReader = current; > firstReaderHoldCount = 1; > - } else if (firstReader == tid) { > + } else if (firstReader == current) { > firstReaderHoldCount++; > } else { > - if (rh == null) { > + if (rh == null) > rh = cachedHoldCounter; > - if (rh != null && rh.tid == tid) { > - if (rh.count == 0) > - readHolds.set(rh); > - } else { > - rh = readHolds.get(); > - } > - } else if (rh.count == 0) > + if (rh == null || rh.tid != current.getId()) > + rh = readHolds.get(); > + else if (rh.count == 0) > readHolds.set(rh); > + rh.count++; > cachedHoldCounter = rh; // cache for release > - rh.count++; > } > return 1; > } > @@ -572,15 +586,14 @@ > if (r == MAX_COUNT) > throw new Error("Maximum lock count exceeded"); > if (compareAndSetState(c, c + SHARED_UNIT)) { > - long tid = current.getId(); > if (r == 0) { > - firstReader = tid; > + firstReader = current; > firstReaderHoldCount = 1; > - } else if (firstReader == tid) { > + } else if (firstReader == current) { > firstReaderHoldCount++; > } else { > HoldCounter rh = cachedHoldCounter; > - if (rh == null || rh.tid != tid) > + if (rh == null || rh.tid != current.getId()) > cachedHoldCounter = rh = readHolds.get(); > else if (rh.count == 0) > readHolds.set(rh); > @@ -626,12 +639,12 @@ > if (getReadLockCount() == 0) > return 0; > > - long tid = Thread.currentThread().getId(); > - if (firstReader == tid) > + Thread current = Thread.currentThread(); > + if (firstReader == current) > return firstReaderHoldCount; > > HoldCounter rh = cachedHoldCounter; > - if (rh != null && rh.tid == tid) > + if (rh != null && rh.tid == current.getId()) > return rh.count; > > int count = readHolds.get().count; > @@ -647,7 +660,6 @@ > throws java.io.IOException, ClassNotFoundException { > s.defaultReadObject(); > readHolds = new ThreadLocalHoldCounter(); > - firstReader = INVALID_THREAD_ID; > setState(0); // reset to unlocked state > }

26-03-2009

EVALUATION See description and suggested fix.

26-03-2009