8187408: AbstractQueuedSynchronizer wait queue corrupted when thread awaits without holding the lock
Reviewed-by: martin, psandoz, dholmes
--- a/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java Tue Oct 03 13:32:04 2017 -0700
+++ b/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java Tue Oct 03 13:37:01 2017 -0700
@@ -67,11 +67,11 @@
private static final long serialVersionUID = 7373984972572414692L;
/*
- To keep sources in sync, the remainder of this source file is
- exactly cloned from AbstractQueuedSynchronizer, replacing class
- name and changing ints related with sync state to longs. Please
- keep it that way.
- */
+ * To keep sources in sync, the remainder of this source file is
+ * exactly cloned from AbstractQueuedSynchronizer, replacing class
+ * name and changing ints related with sync state to longs. Please
+ * keep it that way.
+ */
/**
* Creates a new {@code AbstractQueuedLongSynchronizer} instance
@@ -725,8 +725,7 @@
/**
* Returns {@code true} if synchronization is held exclusively with
* respect to the current (calling) thread. This method is invoked
- * upon each call to a non-waiting {@link ConditionObject} method.
- * (Waiting methods instead invoke {@link #release}.)
+ * upon each call to a {@link ConditionObject} method.
*
* <p>The default implementation throws {@link
* UnsupportedOperationException}. This method is invoked
@@ -1366,9 +1365,8 @@
}
/**
- * Condition implementation for a {@link
- * AbstractQueuedLongSynchronizer} serving as the basis of a {@link
- * Lock} implementation.
+ * Condition implementation for a {@link AbstractQueuedLongSynchronizer}
+ * serving as the basis of a {@link Lock} implementation.
*
* <p>Method documentation for this class describes mechanics,
* not behavioral specifications from the point of view of Lock
@@ -1401,6 +1399,8 @@
* @return its new wait node
*/
private Node addConditionWaiter() {
+ if (!isHeldExclusively())
+ throw new IllegalMonitorStateException();
Node t = lastWaiter;
// If lastWaiter is cancelled, clean out.
if (t != null && t.waitStatus != Node.CONDITION) {
--- a/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java Tue Oct 03 13:32:04 2017 -0700
+++ b/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java Tue Oct 03 13:37:01 2017 -0700
@@ -194,19 +194,13 @@
* represent the locked state. While a non-reentrant lock
* does not strictly require recording of the current owner
* thread, this class does so anyway to make usage easier to monitor.
- * It also supports conditions and exposes
- * one of the instrumentation methods:
+ * It also supports conditions and exposes some instrumentation methods:
*
* <pre> {@code
* class Mutex implements Lock, java.io.Serializable {
*
* // Our internal helper class
* private static class Sync extends AbstractQueuedSynchronizer {
- * // Reports whether in locked state
- * protected boolean isHeldExclusively() {
- * return getState() == 1;
- * }
- *
* // Acquires the lock if state is zero
* public boolean tryAcquire(int acquires) {
* assert acquires == 1; // Otherwise unused
@@ -220,14 +214,27 @@
* // Releases the lock by setting state to zero
* protected boolean tryRelease(int releases) {
* assert releases == 1; // Otherwise unused
- * if (getState() == 0) throw new IllegalMonitorStateException();
+ * if (!isHeldExclusively())
+ * throw new IllegalMonitorStateException();
* setExclusiveOwnerThread(null);
* setState(0);
* return true;
* }
*
+ * // Reports whether in locked state
+ * public boolean isLocked() {
+ * return getState() != 0;
+ * }
+ *
+ * public boolean isHeldExclusively() {
+ * // a data race, but safe due to out-of-thin-air guarantees
+ * return getExclusiveOwnerThread() == Thread.currentThread();
+ * }
+ *
* // Provides a Condition
- * Condition newCondition() { return new ConditionObject(); }
+ * public Condition newCondition() {
+ * return new ConditionObject();
+ * }
*
* // Deserializes properly
* private void readObject(ObjectInputStream s)
@@ -240,12 +247,17 @@
* // The sync object does all the hard work. We just forward to it.
* private final Sync sync = new Sync();
*
- * public void lock() { sync.acquire(1); }
- * public boolean tryLock() { return sync.tryAcquire(1); }
- * public void unlock() { sync.release(1); }
- * public Condition newCondition() { return sync.newCondition(); }
- * public boolean isLocked() { return sync.isHeldExclusively(); }
- * public boolean hasQueuedThreads() { return sync.hasQueuedThreads(); }
+ * public void lock() { sync.acquire(1); }
+ * public boolean tryLock() { return sync.tryAcquire(1); }
+ * public void unlock() { sync.release(1); }
+ * public Condition newCondition() { return sync.newCondition(); }
+ * public boolean isLocked() { return sync.isLocked(); }
+ * public boolean isHeldByCurrentThread() {
+ * return sync.isHeldExclusively();
+ * }
+ * public boolean hasQueuedThreads() {
+ * return sync.hasQueuedThreads();
+ * }
* public void lockInterruptibly() throws InterruptedException {
* sync.acquireInterruptibly(1);
* }
@@ -1193,8 +1205,7 @@
/**
* Returns {@code true} if synchronization is held exclusively with
* respect to the current (calling) thread. This method is invoked
- * upon each call to a non-waiting {@link ConditionObject} method.
- * (Waiting methods instead invoke {@link #release}.)
+ * upon each call to a {@link ConditionObject} method.
*
* <p>The default implementation throws {@link
* UnsupportedOperationException}. This method is invoked
@@ -1834,9 +1845,8 @@
}
/**
- * Condition implementation for a {@link
- * AbstractQueuedSynchronizer} serving as the basis of a {@link
- * Lock} implementation.
+ * Condition implementation for a {@link AbstractQueuedSynchronizer}
+ * serving as the basis of a {@link Lock} implementation.
*
* <p>Method documentation for this class describes mechanics,
* not behavioral specifications from the point of view of Lock
@@ -1867,6 +1877,8 @@
* @return its new wait node
*/
private Node addConditionWaiter() {
+ if (!isHeldExclusively())
+ throw new IllegalMonitorStateException();
Node t = lastWaiter;
// If lastWaiter is cancelled, clean out.
if (t != null && t.waitStatus != Node.CONDITION) {
--- a/test/jdk/java/util/concurrent/tck/AbstractQueuedLongSynchronizerTest.java Tue Oct 03 13:32:04 2017 -0700
+++ b/test/jdk/java/util/concurrent/tck/AbstractQueuedLongSynchronizerTest.java Tue Oct 03 13:37:01 2017 -0700
@@ -58,6 +58,9 @@
/**
* A simple mutex class, adapted from the class javadoc. Exclusive
* acquire tests exercise this as a sample user extension.
+ *
+ * Unlike the javadoc sample, we don't track owner thread via
+ * AbstractOwnableSynchronizer methods.
*/
static class Mutex extends AbstractQueuedLongSynchronizer {
/** An eccentric value > 32 bits for locked synchronizer state. */
@@ -65,18 +68,19 @@
static final long UNLOCKED = 0;
- public boolean isHeldExclusively() {
+ /** Owner thread is untracked, so this is really just isLocked(). */
+ @Override public boolean isHeldExclusively() {
long state = getState();
assertTrue(state == UNLOCKED || state == LOCKED);
return state == LOCKED;
}
- public boolean tryAcquire(long acquires) {
+ @Override protected boolean tryAcquire(long acquires) {
assertEquals(LOCKED, acquires);
return compareAndSetState(UNLOCKED, LOCKED);
}
- public boolean tryRelease(long releases) {
+ @Override protected boolean tryRelease(long releases) {
if (getState() != LOCKED) throw new IllegalMonitorStateException();
setState(UNLOCKED);
return true;
@@ -106,13 +110,14 @@
release(LOCKED);
}
+ /** Faux-Implements Lock.newCondition(). */
public ConditionObject newCondition() {
return new ConditionObject();
}
}
/**
- * A simple latch class, to test shared mode.
+ * A minimal latch class, to test shared mode.
*/
static class BooleanLatch extends AbstractQueuedLongSynchronizer {
public boolean isSignalled() { return getState() != 0; }
--- a/test/jdk/java/util/concurrent/tck/AbstractQueuedSynchronizerTest.java Tue Oct 03 13:32:04 2017 -0700
+++ b/test/jdk/java/util/concurrent/tck/AbstractQueuedSynchronizerTest.java Tue Oct 03 13:37:01 2017 -0700
@@ -61,6 +61,9 @@
* methods/features of AbstractQueuedSynchronizer are tested via
* other test classes, including those for ReentrantLock,
* ReentrantReadWriteLock, and Semaphore.
+ *
+ * Unlike the javadoc sample, we don't track owner thread via
+ * AbstractOwnableSynchronizer methods.
*/
static class Mutex extends AbstractQueuedSynchronizer {
/** An eccentric value for locked synchronizer state. */
@@ -68,18 +71,19 @@
static final int UNLOCKED = 0;
+ /** Owner thread is untracked, so this is really just isLocked(). */
@Override public boolean isHeldExclusively() {
int state = getState();
assertTrue(state == UNLOCKED || state == LOCKED);
return state == LOCKED;
}
- @Override public boolean tryAcquire(int acquires) {
+ @Override protected boolean tryAcquire(int acquires) {
assertEquals(LOCKED, acquires);
return compareAndSetState(UNLOCKED, LOCKED);
}
- @Override public boolean tryRelease(int releases) {
+ @Override protected boolean tryRelease(int releases) {
if (getState() != LOCKED) throw new IllegalMonitorStateException();
assertEquals(LOCKED, releases);
setState(UNLOCKED);
@@ -110,13 +114,14 @@
release(LOCKED);
}
+ /** Faux-Implements Lock.newCondition(). */
public ConditionObject newCondition() {
return new ConditionObject();
}
}
/**
- * A simple latch class, to test shared mode.
+ * A minimal latch class, to test shared mode.
*/
static class BooleanLatch extends AbstractQueuedSynchronizer {
public boolean isSignalled() { return getState() != 0; }
--- a/test/jdk/java/util/concurrent/tck/ReentrantLockTest.java Tue Oct 03 13:32:04 2017 -0700
+++ b/test/jdk/java/util/concurrent/tck/ReentrantLockTest.java Tue Oct 03 13:37:01 2017 -0700
@@ -35,11 +35,13 @@
import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ThreadLocalRandom;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.ReentrantLock;
@@ -173,6 +175,11 @@
enum AwaitMethod { await, awaitTimed, awaitNanos, awaitUntil }
+ static AwaitMethod randomAwaitMethod() {
+ AwaitMethod[] awaitMethods = AwaitMethod.values();
+ return awaitMethods[ThreadLocalRandom.current().nextInt(awaitMethods.length)];
+ }
+
/**
* Awaits condition "indefinitely" using the specified AwaitMethod.
*/
@@ -1160,4 +1167,60 @@
lock.unlock();
assertTrue(lock.toString().contains("Unlocked"));
}
+
+ /**
+ * Tests scenario for JDK-8187408
+ * AbstractQueuedSynchronizer wait queue corrupted when thread awaits without holding the lock
+ */
+ public void testBug8187408() throws InterruptedException {
+ final ThreadLocalRandom rnd = ThreadLocalRandom.current();
+ final AwaitMethod awaitMethod = randomAwaitMethod();
+ final int nThreads = rnd.nextInt(2, 10);
+ final ReentrantLock lock = new ReentrantLock();
+ final Condition cond = lock.newCondition();
+ final CountDownLatch done = new CountDownLatch(nThreads);
+ final ArrayList<Thread> threads = new ArrayList<>();
+
+ Runnable rogue = () -> {
+ while (done.getCount() > 0) {
+ try {
+ // call await without holding lock?!
+ await(cond, awaitMethod);
+ throw new AssertionError("should throw");
+ }
+ catch (IllegalMonitorStateException expected) {}
+ catch (Throwable fail) { threadUnexpectedException(fail); }}};
+ Thread rogueThread = new Thread(rogue, "rogue");
+ threads.add(rogueThread);
+ rogueThread.start();
+
+ Runnable waiter = () -> {
+ lock.lock();
+ try {
+ done.countDown();
+ cond.await();
+ } catch (Throwable fail) {
+ threadUnexpectedException(fail);
+ } finally {
+ lock.unlock();
+ }};
+ for (int i = 0; i < nThreads; i++) {
+ Thread thread = new Thread(waiter, "waiter");
+ threads.add(thread);
+ thread.start();
+ }
+
+ assertTrue(done.await(LONG_DELAY_MS, MILLISECONDS));
+ lock.lock();
+ try {
+ assertEquals(nThreads, lock.getWaitQueueLength(cond));
+ } finally {
+ cond.signalAll();
+ lock.unlock();
+ }
+ for (Thread thread : threads) {
+ thread.join(LONG_DELAY_MS);
+ assertFalse(thread.isAlive());
+ }
+ }
}