8187408: AbstractQueuedSynchronizer wait queue corrupted when thread awaits without holding the lock
authordl
Tue, 03 Oct 2017 13:37:01 -0700
changeset 47302 f517fa4f4dc6
parent 47301 14a82b038e5a
child 47303 e0637258a133
8187408: AbstractQueuedSynchronizer wait queue corrupted when thread awaits without holding the lock Reviewed-by: martin, psandoz, dholmes
src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java
src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java
test/jdk/java/util/concurrent/tck/AbstractQueuedLongSynchronizerTest.java
test/jdk/java/util/concurrent/tck/AbstractQueuedSynchronizerTest.java
test/jdk/java/util/concurrent/tck/ReentrantLockTest.java
--- 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());
+        }
+    }
 }