From 9f6e06dc439d6f27b44fc6a309f7b42471f460cf Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Tue, 21 Apr 2015 12:24:43 +0200 Subject: [PATCH] [#3675] Fix livelock issue in MpscLinkedQueue Motivation: All read operations should be safe to execute from multiple threads which was not the case and so could produce a livelock. Modifications: Modify methods so these are safe to be called from multiple threads. Result: No more livelock. --- .../netty/util/internal/MpscLinkedQueue.java | 131 ++++++++---------- 1 file changed, 59 insertions(+), 72 deletions(-) diff --git a/common/src/main/java/io/netty/util/internal/MpscLinkedQueue.java b/common/src/main/java/io/netty/util/internal/MpscLinkedQueue.java index 4baa5ac0fb..c1b931a562 100644 --- a/common/src/main/java/io/netty/util/internal/MpscLinkedQueue.java +++ b/common/src/main/java/io/netty/util/internal/MpscLinkedQueue.java @@ -21,10 +21,10 @@ package io.netty.util.internal; import java.io.IOException; import java.io.ObjectInputStream; import java.io.ObjectOutputStream; -import java.lang.reflect.Array; -import java.util.Arrays; +import java.util.ArrayList; import java.util.Collection; import java.util.Iterator; +import java.util.List; import java.util.NoSuchElementException; import java.util.Queue; @@ -164,12 +164,20 @@ final class MpscLinkedQueue extends MpscLinkedQueueTailRef implements Queu public int size() { int count = 0; MpscLinkedQueueNode n = peekNode(); - for (;;) { - if (n == null) { + for (;;) { + // If value == null it means that clearMaybe() was called on the MpscLinkedQueueNode. + if (n == null || n.value() == null) { + break; + } + MpscLinkedQueueNode next = n.next(); + if (n == next) { + break; + } + n = next; + if (++ count == Integer.MAX_VALUE) { + // Guard against overflow of integer. break; } - count ++; - n = n.next(); } return count; } @@ -186,40 +194,26 @@ final class MpscLinkedQueue extends MpscLinkedQueueTailRef implements Queu if (n == null) { break; } - if (n.value() == o) { + E value = n.value(); + // If value == null it means that clearMaybe() was called on the MpscLinkedQueueNode. + if (value == null) { + return false; + } + if (value == o) { return true; } - n = n.next(); + MpscLinkedQueueNode next = n.next(); + if (n == next) { + break; + } + n = next; } return false; } @Override public Iterator iterator() { - return new Iterator() { - private MpscLinkedQueueNode node = peekNode(); - - @Override - public boolean hasNext() { - return node != null; - } - - @Override - public E next() { - MpscLinkedQueueNode node = this.node; - if (node == null) { - throw new NoSuchElementException(); - } - E value = node.value(); - this.node = node.next(); - return value; - } - - @Override - public void remove() { - throw new UnsupportedOperationException(); - } - }; + return new ReadOnlyIterator(toList().iterator()); } @Override @@ -248,53 +242,46 @@ final class MpscLinkedQueue extends MpscLinkedQueueTailRef implements Queu throw new NoSuchElementException(); } + private List toList(int initialCapacity) { + return toList(new ArrayList(initialCapacity)); + } + + private List toList() { + return toList(new ArrayList()); + } + + private List toList(List elements) { + MpscLinkedQueueNode n = peekNode(); + for (;;) { + if (n == null) { + break; + } + E value = n.value(); + if (value == null) { + break; + } + if (!elements.add(value)) { + // Seems like there is no space left, break here. + break; + } + MpscLinkedQueueNode next = n.next(); + if (n == next) { + break; + } + n = next; + } + return elements; + } + @Override public Object[] toArray() { - final Object[] array = new Object[size()]; - final Iterator it = iterator(); - for (int i = 0; i < array.length; i ++) { - if (it.hasNext()) { - array[i] = it.next(); - } else { - return Arrays.copyOf(array, i); - } - } - return array; + return toList().toArray(); } @Override @SuppressWarnings("unchecked") public T[] toArray(T[] a) { - final int size = size(); - final T[] array; - if (a.length >= size) { - array = a; - } else { - array = (T[]) Array.newInstance(a.getClass().getComponentType(), size); - } - - final Iterator it = iterator(); - for (int i = 0; i < array.length; i++) { - if (it.hasNext()) { - array[i] = (T) it.next(); - } else { - if (a == array) { - array[i] = null; - return array; - } - - if (a.length < i) { - return Arrays.copyOf(array, i); - } - - System.arraycopy(array, 0, a, 0, i); - if (a.length > i) { - a[i] = null; - } - return a; - } - } - return array; + return toList(a.length).toArray(a); } @Override