6993789: LinkedBlockingDeque iterator/descendingIterator loops and owns lock forever
Reviewed-by: dl, dholmes
--- a/jdk/src/share/classes/java/util/concurrent/LinkedBlockingDeque.java Mon Nov 15 14:34:04 2010 +0000
+++ b/jdk/src/share/classes/java/util/concurrent/LinkedBlockingDeque.java Mon Nov 15 15:11:04 2010 +0000
@@ -126,10 +126,8 @@
*/
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 @@
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 @@
// 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);
- first = x;
+ node.next = f;
+ 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);
- last = x;
+ node.prev = l;
+ 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 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 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 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 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 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 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 @@
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();
}
@@ -1054,6 +1071,26 @@
}
/**
+ * 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.
*/
void advance() {
@@ -1061,16 +1098,7 @@
lock.lock();
try {
// assert next != null;
- Node<E> s = nextNode(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;
- }
+ next = succ(next);
nextItem = (next == null) ? null : next.item;
} finally {
lock.unlock();
--- a/jdk/test/java/util/concurrent/ConcurrentQueues/IteratorWeakConsistency.java Mon Nov 15 14:34:04 2010 +0000
+++ b/jdk/test/java/util/concurrent/ConcurrentQueues/IteratorWeakConsistency.java Mon Nov 15 15:11:04 2010 +0000
@@ -56,23 +56,44 @@
// 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++)
- q.add(i);
- Iterator it = q.iterator();
- q.poll();
- q.poll();
- q.poll();
- q.remove(7);
- List list = new ArrayList();
- while (it.hasNext())
- list.add(it.next());
- equal(list, Arrays.asList(0, 3, 4, 5, 6, 8, 9));
- check(! list.contains(null));
- System.out.printf("%s: %s%n",
- q.getClass().getSimpleName(),
- list);
+ try {
+ for (int i = 0; i < 10; i++)
+ q.add(i);
+ Iterator it = q.iterator();
+ q.poll();
+ q.poll();
+ q.poll();
+ q.remove(7);
+ List list = new ArrayList();
+ while (it.hasNext())
+ list.add(it.next());
+ equal(list, Arrays.asList(0, 3, 4, 5, 6, 8, 9));
+ check(! list.contains(null));
+ System.out.printf("%s: %s%n",
+ 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 @@
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 {