8187408: AbstractQueuedSynchronizer wait queue corrupted when thread awaits without holding the lock

Reviewed-by: martin, psandoz, dholmes
This commit is contained in:
Doug Lea 2017-10-03 13:37:01 -07:00
parent dfce305868
commit c6e3667228
5 changed files with 122 additions and 37 deletions

@ -67,11 +67,11 @@ public abstract class AbstractQueuedLongSynchronizer
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 @@ public abstract class AbstractQueuedLongSynchronizer
/**
* 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 @@ public abstract class AbstractQueuedLongSynchronizer
}
/**
* 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 @@ public abstract class AbstractQueuedLongSynchronizer
* @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) {

@ -194,19 +194,13 @@ import java.util.concurrent.TimeUnit;
* 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 @@ import java.util.concurrent.TimeUnit;
* // 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 @@ import java.util.concurrent.TimeUnit;
* // 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 @@ public abstract class AbstractQueuedSynchronizer
/**
* 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 @@ public abstract class AbstractQueuedSynchronizer
}
/**
* 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 @@ public abstract class AbstractQueuedSynchronizer
* @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) {

@ -58,6 +58,9 @@ public class AbstractQueuedLongSynchronizerTest extends JSR166TestCase {
/**
* 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 @@ public class AbstractQueuedLongSynchronizerTest extends JSR166TestCase {
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 @@ public class AbstractQueuedLongSynchronizerTest extends JSR166TestCase {
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; }

@ -61,6 +61,9 @@ public class AbstractQueuedSynchronizerTest extends JSR166TestCase {
* 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 @@ public class AbstractQueuedSynchronizerTest extends JSR166TestCase {
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 @@ public class AbstractQueuedSynchronizerTest extends JSR166TestCase {
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; }

@ -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 @@ public class ReentrantLockTest extends JSR166TestCase {
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 @@ public class ReentrantLockTest extends JSR166TestCase {
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());
}
}
}