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 google#2658 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=140382230
wy65701436 · Nov 28, 2016 · 2ef9551 · 2ef9551
1 parent 16e3f26
commit 2ef9551
Show file tree

Hide file tree

Showing 4 changed files with 108 additions and 8 deletions.
diff --git a/cycle_whitelist.txt b/cycle_whitelist.txt
@@ -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

diff --git a/guava-gwt/test/com/google/common/collect/MinMaxPriorityQueueTest_gwt.java b/guava-gwt/test/com/google/common/collect/MinMaxPriorityQueueTest_gwt.java
@@ -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();

diff --git a/guava-tests/test/com/google/common/collect/MinMaxPriorityQueueTest.java b/guava-tests/test/com/google/common/collect/MinMaxPriorityQueueTest.java
@@ -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.
    */

diff --git a/guava/src/com/google/common/collect/MinMaxPriorityQueue.java b/guava/src/com/google/common/collect/MinMaxPriorityQueue.java
@@ -418,7 +418,14 @@ MoveDesc<E> removeAt(int index) {
       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--;