8191483: AbstractQueuedSynchronizer cancel/cancel race
authordl
Tue, 16 Jan 2018 18:24:32 -0800
changeset 48540 221cf8307606
parent 48539 20ed1cebe5f8
child 48541 946e34c2dec9
8191483: AbstractQueuedSynchronizer cancel/cancel race Reviewed-by: martin
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/AbstractQueuedSynchronizerTest.java
--- a/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java	Wed Jan 17 07:55:20 2018 +0800
+++ b/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java	Tue Jan 16 18:24:32 2018 -0800
@@ -320,7 +320,9 @@
 
         // predNext is the apparent node to unsplice. CASes below will
         // fail if not, in which case, we lost race vs another cancel
-        // or signal, so no further action is necessary.
+        // or signal, so no further action is necessary, although with
+        // a possibility that a cancelled node may transiently remain
+        // reachable.
         Node predNext = pred.next;
 
         // Can use unconditional write instead of CAS here.
@@ -912,13 +914,13 @@
      * at any time, a {@code true} return does not guarantee that any
      * other thread will ever acquire.
      *
-     * <p>In this implementation, this operation returns in
-     * constant time.
-     *
      * @return {@code true} if there may be other threads waiting to acquire
      */
     public final boolean hasQueuedThreads() {
-        return head != tail;
+        for (Node p = tail, h = head; p != h && p != null; p = p.prev)
+            if (p.waitStatus <= 0)
+                return true;
+        return false;
     }
 
     /**
@@ -1067,17 +1069,21 @@
      * @since 1.7
      */
     public final boolean hasQueuedPredecessors() {
-        // The correctness of this depends on head being initialized
-        // before tail and on head.next being accurate if the current
-        // thread is first in queue.
-        Node t = tail; // Read fields in reverse initialization order
-        Node h = head;
-        Node s;
-        return h != t &&
-            ((s = h.next) == null || s.thread != Thread.currentThread());
+        Node h, s;
+        if ((h = head) != null) {
+            if ((s = h.next) == null || s.waitStatus > 0) {
+                s = null; // traverse in case of concurrent cancellation
+                for (Node p = tail; p != h && p != null; p = p.prev) {
+                    if (p.waitStatus <= 0)
+                        s = p;
+                }
+            }
+            if (s != null && s.thread != Thread.currentThread())
+                return true;
+        }
+        return false;
     }
 
-
     // Instrumentation and monitoring methods
 
     /**
--- a/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java	Wed Jan 17 07:55:20 2018 +0800
+++ b/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java	Tue Jan 16 18:24:32 2018 -0800
@@ -800,7 +800,9 @@
 
         // predNext is the apparent node to unsplice. CASes below will
         // fail if not, in which case, we lost race vs another cancel
-        // or signal, so no further action is necessary.
+        // or signal, so no further action is necessary, although with
+        // a possibility that a cancelled node may transiently remain
+        // reachable.
         Node predNext = pred.next;
 
         // Can use unconditional write instead of CAS here.
@@ -1392,13 +1394,13 @@
      * at any time, a {@code true} return does not guarantee that any
      * other thread will ever acquire.
      *
-     * <p>In this implementation, this operation returns in
-     * constant time.
-     *
      * @return {@code true} if there may be other threads waiting to acquire
      */
     public final boolean hasQueuedThreads() {
-        return head != tail;
+        for (Node p = tail, h = head; p != h && p != null; p = p.prev)
+            if (p.waitStatus <= 0)
+                return true;
+        return false;
     }
 
     /**
@@ -1547,17 +1549,21 @@
      * @since 1.7
      */
     public final boolean hasQueuedPredecessors() {
-        // The correctness of this depends on head being initialized
-        // before tail and on head.next being accurate if the current
-        // thread is first in queue.
-        Node t = tail; // Read fields in reverse initialization order
-        Node h = head;
-        Node s;
-        return h != t &&
-            ((s = h.next) == null || s.thread != Thread.currentThread());
+        Node h, s;
+        if ((h = head) != null) {
+            if ((s = h.next) == null || s.waitStatus > 0) {
+                s = null; // traverse in case of concurrent cancellation
+                for (Node p = tail; p != h && p != null; p = p.prev) {
+                    if (p.waitStatus <= 0)
+                        s = p;
+                }
+            }
+            if (s != null && s.thread != Thread.currentThread())
+                return true;
+        }
+        return false;
     }
 
-
     // Instrumentation and monitoring methods
 
     /**
--- a/test/jdk/java/util/concurrent/tck/AbstractQueuedSynchronizerTest.java	Wed Jan 17 07:55:20 2018 +0800
+++ b/test/jdk/java/util/concurrent/tck/AbstractQueuedSynchronizerTest.java	Tue Jan 16 18:24:32 2018 -0800
@@ -1289,11 +1289,10 @@
     }
 
     /**
-     * Disabled demo test for (unfixed as of 2017-11)
      * JDK-8191483: AbstractQueuedSynchronizer cancel/cancel race
      * ant -Djsr166.tckTestClass=AbstractQueuedSynchronizerTest -Djsr166.methodFilter=testCancelCancelRace -Djsr166.runsPerTest=100 tck
      */
-    public void DISABLED_testCancelCancelRace() throws InterruptedException {
+    public void testCancelCancelRace() throws InterruptedException {
         class Sync extends AbstractQueuedSynchronizer {
             protected boolean tryAcquire(int acquires) {
                 return !hasQueuedPredecessors() && compareAndSetState(0, 1);