Permalink
Browse files

Fix two bugs in MinMaxPriorityQueue (introduced in [] First is a bug …

…in removeAt(int) that sometimes causes the wrong element to be removed. Second is a bug that sometimes causes certain elements to be iterated over more than once if elements were removed during iteration.

Reported externally at #2658

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=140382230
  • Loading branch information...
Bezier89 authored and cpovirk committed Nov 28, 2016
1 parent 16e3f26 commit 2ef955163b3d43e7849c1929ef4e5d714b93da96
View
@@ -36,7 +36,7 @@ FIELD com.google.common.collect.Maps.FilteredMapValues.unfiltered
FIELD com.google.common.collect.Sets.SubSet.inputSet
FIELD com.google.common.collect.TreeTraverser.PostOrderNode.childIterator
FIELD com.google.common.collect.TreeTraverser.PreOrderIterator.stack
-FIELD com.google.common.util.concurrent.AbstractFuture.Listener.task
+FIELD com.google.common.util.concurrent.AbstractFuture.Listener.executor com.google.common.util.concurrent.MoreExecutors.rejectionPropagatingExecutor.$
FIELD com.google.common.util.concurrent.AbstractService.listeners
# Real cycle, but the runningState field is null'ed on completion of the future.
FIELD com.google.common.util.concurrent.AggregateFuture.runningState
@@ -188,6 +188,16 @@ public void testIteratorTesterLarger() throws Exception {
testCase.testIteratorTesterLarger();
}
+public void testRandomAddsAndRemoves() throws Exception {
+ com.google.common.collect.MinMaxPriorityQueueTest testCase = new com.google.common.collect.MinMaxPriorityQueueTest();
+ testCase.testRandomAddsAndRemoves();
+}
+
+public void testRandomRemoves() throws Exception {
+ com.google.common.collect.MinMaxPriorityQueueTest testCase = new com.google.common.collect.MinMaxPriorityQueueTest();
+ testCase.testRandomRemoves();
+}
+
public void testRegression_dataCorruption() throws Exception {
com.google.common.collect.MinMaxPriorityQueueTest testCase = new com.google.common.collect.MinMaxPriorityQueueTest();
testCase.testRegression_dataCorruption();
@@ -213,6 +223,11 @@ public void testRemoveFromStringHeap() throws Exception {
testCase.testRemoveFromStringHeap();
}
+public void testRemoveRegression() throws Exception {
+ com.google.common.collect.MinMaxPriorityQueueTest testCase = new com.google.common.collect.MinMaxPriorityQueueTest();
+ testCase.testRemoveRegression();
+}
+
public void testSmall() throws Exception {
com.google.common.collect.MinMaxPriorityQueueTest testCase = new com.google.common.collect.MinMaxPriorityQueueTest();
testCase.testSmall();
@@ -22,6 +22,10 @@
import com.google.common.annotations.GwtIncompatible;
import com.google.common.collect.testing.IteratorFeature;
import com.google.common.collect.testing.IteratorTester;
+import com.google.common.collect.testing.QueueTestSuiteBuilder;
+import com.google.common.collect.testing.TestStringQueueGenerator;
+import com.google.common.collect.testing.features.CollectionFeature;
+import com.google.common.collect.testing.features.CollectionSize;
import com.google.common.testing.NullPointerTester;
import java.util.ArrayList;
import java.util.Arrays;
@@ -33,10 +37,13 @@
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.PriorityQueue;
+import java.util.Queue;
import java.util.Random;
import java.util.SortedMap;
import java.util.concurrent.atomic.AtomicInteger;
+import junit.framework.Test;
import junit.framework.TestCase;
+import junit.framework.TestSuite;
/**
* Unit test for {@link MinMaxPriorityQueue}.
@@ -46,7 +53,23 @@
*/
@GwtCompatible(emulated = true)
public class MinMaxPriorityQueueTest extends TestCase {
- private Ordering<Integer> SOME_COMPARATOR = Ordering.natural().reverse();
+ private static final Ordering<Integer> SOME_COMPARATOR = Ordering.natural().reverse();
+
+ @GwtIncompatible // suite
+ public static Test suite() {
+ TestSuite suite = new TestSuite();
+ suite.addTestSuite(MinMaxPriorityQueueTest.class);
+ suite.addTest(QueueTestSuiteBuilder
+ .using(new TestStringQueueGenerator() {
+ @Override protected Queue<String> create(String[] elements) {
+ return MinMaxPriorityQueue.create(Arrays.asList(elements));
+ }
+ })
+ .named("MinMaxPriorityQueue")
+ .withFeatures(CollectionSize.ANY, CollectionFeature.GENERAL_PURPOSE)
+ .createTestSuite());
+ return suite;
+ }
// Overkill alert! Test all combinations of 0-2 options during creation.
@@ -226,8 +249,8 @@ public void testHeapIntact() {
}
}
assertEquals(currentHeapSize, mmHeap.size());
- assertTrue("Heap not intact after " + numberOfModifications +
- " random mixture of operations", mmHeap.isIntact());
+ assertTrue("Heap not intact after " + numberOfModifications
+ + " random mixture of operations", mmHeap.isIntact());
}
public void testSmall() {
@@ -742,6 +765,58 @@ public void testRegression_dataCorruption() {
assertEquals(expected, elements);
}
+ /**
+ * Regression test for https://github.com/google/guava/issues/2658
+ */
+ public void testRemoveRegression() {
+ MinMaxPriorityQueue<Long> queue =
+ MinMaxPriorityQueue.create(ImmutableList.of(2L, 3L, 0L, 4L, 1L));
+ queue.remove(4L);
+ queue.remove(1L);
+ assertThat(queue).doesNotContain(1L);
+ }
+
+ public void testRandomRemoves() {
+ Random random = new Random(0);
+ for (int attempts = 0; attempts < 1000; attempts++) {
+ ArrayList<Integer> elements = createOrderedList(10);
+ Collections.shuffle(elements, random);
+ MinMaxPriorityQueue<Integer> queue = MinMaxPriorityQueue.create(elements);
+ Collections.shuffle(elements, random);
+ for (Integer element : elements) {
+ assertThat(queue.remove(element)).isTrue();
+ assertThat(queue.isIntact()).isTrue();
+ assertThat(queue).doesNotContain(element);
+ }
+ assertThat(queue).isEmpty();
+ }
+ }
+
+ public void testRandomAddsAndRemoves() {
+ Random random = new Random(0);
+ Multiset<Integer> elements = HashMultiset.create();
+ MinMaxPriorityQueue<Integer> queue = MinMaxPriorityQueue.create();
+ int range = 10_000; // range should be small enough that equal elements occur semi-frequently
+ for (int iter = 0; iter < 1000; iter++) {
+ for (int i = 0; i < 100; i++) {
+ Integer element = random.nextInt(range);
+ elements.add(element);
+ queue.add(element);
+ }
+ Iterator<Integer> queueIterator = queue.iterator();
+ while (queueIterator.hasNext()) {
+ Integer element = queueIterator.next();
+ assertThat(elements).contains(element);
+ if (random.nextBoolean()) {
+ elements.remove(element);
+ queueIterator.remove();
+ }
+ }
+ assertThat(queue.isIntact()).isTrue();
+ assertThat(queue).containsExactlyElementsIn(elements);
+ }
+ }
+
/**
* Returns the seed used for the randomization.
*/
@@ -418,7 +418,14 @@ public E peekLast() {
return null;
}
E actualLastElement = elementData(size);
- int lastElementAt = heapForIndex(size).getCorrectLastElement(actualLastElement);
+ int lastElementAt = heapForIndex(size).swapWithConceptuallyLastElement(actualLastElement);
+ if (lastElementAt == index) {
+ // 'actualLastElement' is now at 'lastElementAt', and the element that was at 'lastElementAt'
+ // is now at the end of queue. If that's the element we wanted to remove in the first place,
+ // don't try to (incorrectly) trickle it. Instead, just delete it and we're done.
+ queue[size] = null;
+ return null;
+ }
E toTrickle = elementData(size);
queue[size] = null;
MoveDesc<E> changes = fillHole(index, toTrickle);
@@ -669,15 +676,16 @@ int crossOverUp(int index, E x) {
}
/**
- * Returns the conceptually correct last element of the heap.
+ * Swap {@code actualLastElement} with the conceptually correct last element of the heap.
+ * Returns the index that {@code actualLastElement} now resides in.
*
* <p>Since the last element of the array is actually in the
* middle of the sorted structure, a childless uncle node could be
* smaller, which would corrupt the invariant if this element
* becomes the new parent of the uncle. In that case, we first
* switch the last element with its uncle, before returning.
*/
- int getCorrectLastElement(E actualLastElement) {
+ int swapWithConceptuallyLastElement(E actualLastElement) {
int parentIndex = getParentIndex(size);
if (parentIndex != 0) {
int grandparentIndex = getParentIndex(parentIndex);
@@ -817,7 +825,9 @@ public void remove() {
forgetMeNot = new ArrayDeque<E>();
skipMe = new ArrayList<E>(3);
}
- forgetMeNot.add(moved.toTrickle);
+ if (!containsExact(skipMe, moved.toTrickle)) {
+ forgetMeNot.add(moved.toTrickle);
+ }
skipMe.add(moved.replaced);
}
cursor--;

0 comments on commit 2ef9551

Please sign in to comment.