6993789: LinkedBlockingDeque iterator/descendingIterator loops and owns lock forever

Reviewed-by: dl, dholmes

jdk/src/share/classes/java/util/concurrent/LinkedBlockingDeque.java

@@ -126,10 +126,8 @@ public class LinkedBlockingDeque<E>
          */
         Node<E> next;
 
-        Node(E x, Node<E> p, Node<E> n) {
+        Node(E x) {
             item = x;
-            prev = p;
-            next = n;
         }
     }
 
@@ -199,7 +197,7 @@ public class LinkedBlockingDeque<E>
             for (E e : c) {
                 if (e == null)
                     throw new NullPointerException();
-                if (!linkLast(e))
+                if (!linkLast(new Node<E>(e)))
                     throw new IllegalStateException("Deque full");
             }
         } finally {
@@ -211,38 +209,38 @@ public class LinkedBlockingDeque<E>
     // Basic linking and unlinking operations, called only while holding lock
 
     /**
-     * Links e as first element, or returns false if full.
+     * Links node as first element, or returns false if full.
      */
-    private boolean linkFirst(E e) {
+    private boolean linkFirst(Node<E> node) {
         // assert lock.isHeldByCurrentThread();
         if (count >= capacity)
             return false;
         Node<E> f = first;
-        Node<E> x = new Node<E>(e, null, f);
+        node.next = f;
-        first = x;
+        first = node;
         if (last == null)
-            last = x;
+            last = node;
         else
-            f.prev = x;
+            f.prev = node;
         ++count;
         notEmpty.signal();
         return true;
     }
 
     /**
-     * Links e as last element, or returns false if full.
+     * Links node as last element, or returns false if full.
      */
-    private boolean linkLast(E e) {
+    private boolean linkLast(Node<E> node) {
         // assert lock.isHeldByCurrentThread();
         if (count >= capacity)
             return false;
         Node<E> l = last;
-        Node<E> x = new Node<E>(e, l, null);
+        node.prev = l;
-        last = x;
+        last = node;
         if (first == null)
-            first = x;
+            first = node;
         else
-            l.next = x;
+            l.next = node;
         ++count;
         notEmpty.signal();
         return true;
@@ -339,10 +337,11 @@ public class LinkedBlockingDeque<E>
      */
     public boolean offerFirst(E e) {
         if (e == null) throw new NullPointerException();
+        Node<E> node = new Node<E>(e);
         final ReentrantLock lock = this.lock;
         lock.lock();
         try {
-            return linkFirst(e);
+            return linkFirst(node);
         } finally {
             lock.unlock();
         }
@@ -353,10 +352,11 @@ public class LinkedBlockingDeque<E>
      */
     public boolean offerLast(E e) {
         if (e == null) throw new NullPointerException();
+        Node<E> node = new Node<E>(e);
         final ReentrantLock lock = this.lock;
         lock.lock();
         try {
-            return linkLast(e);
+            return linkLast(node);
         } finally {
             lock.unlock();
         }
@@ -368,10 +368,11 @@ public class LinkedBlockingDeque<E>
      */
     public void putFirst(E e) throws InterruptedException {
         if (e == null) throw new NullPointerException();
+        Node<E> node = new Node<E>(e);
         final ReentrantLock lock = this.lock;
         lock.lock();
         try {
-            while (!linkFirst(e))
+            while (!linkFirst(node))
                 notFull.await();
         } finally {
             lock.unlock();
@@ -384,10 +385,11 @@ public class LinkedBlockingDeque<E>
      */
     public void putLast(E e) throws InterruptedException {
         if (e == null) throw new NullPointerException();
+        Node<E> node = new Node<E>(e);
         final ReentrantLock lock = this.lock;
         lock.lock();
         try {
-            while (!linkLast(e))
+            while (!linkLast(node))
                 notFull.await();
         } finally {
             lock.unlock();
@@ -401,11 +403,12 @@ public class LinkedBlockingDeque<E>
     public boolean offerFirst(E e, long timeout, TimeUnit unit)
         throws InterruptedException {
         if (e == null) throw new NullPointerException();
+        Node<E> node = new Node<E>(e);
         long nanos = unit.toNanos(timeout);
         final ReentrantLock lock = this.lock;
         lock.lockInterruptibly();
         try {
-            while (!linkFirst(e)) {
+            while (!linkFirst(node)) {
                 if (nanos <= 0)
                     return false;
                 nanos = notFull.awaitNanos(nanos);
@@ -423,11 +426,12 @@ public class LinkedBlockingDeque<E>
     public boolean offerLast(E e, long timeout, TimeUnit unit)
         throws InterruptedException {
         if (e == null) throw new NullPointerException();
+        Node<E> node = new Node<E>(e);
         long nanos = unit.toNanos(timeout);
         final ReentrantLock lock = this.lock;
         lock.lockInterruptibly();
         try {
-            while (!linkLast(e)) {
+            while (!linkLast(node)) {
                 if (nanos <= 0)
                     return false;
                 nanos = notFull.awaitNanos(nanos);
@@ -955,7 +959,20 @@ public class LinkedBlockingDeque<E>
         final ReentrantLock lock = this.lock;
         lock.lock();
         try {
-            return super.toString();
+            Node<E> p = first;
+            if (p == null)
+                return "[]";
+
+            StringBuilder sb = new StringBuilder();
+            sb.append('[');
+            for (;;) {
+                E e = p.item;
+                sb.append(e == this ? "(this Collection)" : e);
+                p = p.next;
+                if (p == null)
+                    return sb.append(']').toString();
+                sb.append(',').append(' ');
+            }
         } finally {
             lock.unlock();
         }
@@ -1053,6 +1070,26 @@ public class LinkedBlockingDeque<E>
             }
         }
 
+        /**
+         * Returns the successor node of the given non-null, but
+         * possibly previously deleted, node.
+         */
+        private Node<E> succ(Node<E> n) {
+            // Chains of deleted nodes ending in null or self-links
+            // are possible if multiple interior nodes are removed.
+            for (;;) {
+                Node<E> s = nextNode(n);
+                if (s == null)
+                    return null;
+                else if (s.item != null)
+                    return s;
+                else if (s == n)
+                    return firstNode();
+                else
+                    n = s;
+            }
+        }
+
         /**
          * Advances next.
          */
@@ -1061,16 +1098,7 @@ public class LinkedBlockingDeque<E>
             lock.lock();
             try {
                 // assert next != null;
-                Node<E> s = nextNode(next);
+                next = succ(next);
-                if (s == next) {
-                    next = firstNode();
-                } else {
-                    // Skip over removed nodes.
-                    // May be necessary if multiple interior Nodes are removed.
-                    while (s != null && s.item == null)
-                        s = nextNode(s);
-                    next = s;
-                }
                 nextItem = (next == null) ? null : next.item;
             } finally {
                 lock.unlock();

jdk/test/java/util/concurrent/ConcurrentQueues/IteratorWeakConsistency.java

@@ -56,23 +56,44 @@ public class IteratorWeakConsistency {
         // test(new ArrayBlockingQueue(20));
     }
 
-    void test(Queue q) throws Throwable {
+    void test(Queue q) {
         // TODO: make this more general
-        for (int i = 0; i < 10; i++)
+        try {
-            q.add(i);
+            for (int i = 0; i < 10; i++)
-        Iterator it = q.iterator();
+                q.add(i);
-        q.poll();
+            Iterator it = q.iterator();
-        q.poll();
+            q.poll();
-        q.poll();
+            q.poll();
-        q.remove(7);
+            q.poll();
-        List list = new ArrayList();
+            q.remove(7);
-        while (it.hasNext())
+            List list = new ArrayList();
-            list.add(it.next());
+            while (it.hasNext())
-        equal(list, Arrays.asList(0, 3, 4, 5, 6, 8, 9));
+                list.add(it.next());
-        check(! list.contains(null));
+            equal(list, Arrays.asList(0, 3, 4, 5, 6, 8, 9));
-        System.out.printf("%s: %s%n",
+            check(! list.contains(null));
-                          q.getClass().getSimpleName(),
+            System.out.printf("%s: %s%n",
-                          list);
+                              q.getClass().getSimpleName(),
+                              list);
+        } catch (Throwable t) { unexpected(t); }
+
+        try {
+            q.clear();
+            q.add(1);
+            q.add(2);
+            q.add(3);
+            q.add(4);
+            Iterator it = q.iterator();
+            it.next();
+            q.remove(2);
+            q.remove(1);
+            q.remove(3);
+            boolean found4 = false;
+            while (it.hasNext()) {
+                found4 |= it.next().equals(4);
+            }
+            check(found4);
+        } catch (Throwable t) { unexpected(t); }
+
     }
 
     //--------------------- Infrastructure ---------------------------
@@ -85,7 +106,6 @@ public class IteratorWeakConsistency {
     void equal(Object x, Object y) {
         if (x == null ? y == null : x.equals(y)) pass();
         else fail(x + " not equal to " + y);}
-    static Class<?> thisClass = new Object(){}.getClass().getEnclosingClass();
     public static void main(String[] args) throws Throwable {
         new IteratorWeakConsistency().instanceMain(args);}
     public void instanceMain(String[] args) throws Throwable {