[collections-838] hasNext() value is now cached till next() or remove…

src/changes/changes.xml

@@ -32,6 +32,7 @@
     <action type="fix" dev="ggregory" due-to="Sebastian Götz, Gary Gregory" issue="COLLECTIONS-874">MapUtils.getLongValue(Map, K, Function) returns a byte instead of a long.</action>
     <action type="fix" dev="ggregory" due-to="Gary Gregory">Fix exception message in org.apache.commons.collections4.functors.FunctorUtils.validate(Consumer...)</action>
     <action type="fix" dev="ggregory" due-to="Gary Gregory">Fix exception message in org.apache.commons.collections4.iterators.UnmodifiableIterator.remove() to match java.util.Iterator.remove().</action>
+    <action type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz, Joerg Budischewski" issue="COLLECTIONS-838">Calling SetUtils.union on multiple instances of SetView causes JVM to hang</action>
     <!-- ADD -->
     <action type="add" dev="ggregory" due-to="Gary Gregory">Add generics to UnmodifiableIterator for the wrapped type.</action>
     <!-- UPDATE -->

src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java

@@ -63,7 +63,7 @@ public class IteratorChain<E> implements Iterator<E> {
     private Iterator<? extends E> currentIterator;
 
     /**
-     * The "last used" Iterator is the Iterator upon which next() or hasNext()
+     * The "last used" Iterator is the Iterator upon which next()
      * was most recently called used for the remove() operation only
      */
     private Iterator<? extends E> lastUsedIterator;
@@ -74,6 +74,11 @@ public class IteratorChain<E> implements Iterator<E> {
      */
     private boolean isLocked;
 
+    /**
+     * Contains the result of the last hasNext() call until next() is invoked
+     */
+    private Boolean cachedHasNextValue;
+
     /**
      * Constructs an IteratorChain with no Iterators.
      * <p>
@@ -183,9 +188,10 @@ private void checkLocked() {
     @Override
     public boolean hasNext() {
         lockChain();
-        updateCurrentIterator();
-        lastUsedIterator = currentIterator;
-        return currentIterator.hasNext();
+        if (cachedHasNextValue == null) {
+            updateCurrentIterator();
+        }
+        return cachedHasNextValue;
     }
 
     /**
@@ -219,9 +225,11 @@ private void lockChain() {
     @Override
     public E next() {
         lockChain();
-        updateCurrentIterator();
+        if (cachedHasNextValue == null) {
+            updateCurrentIterator();
+        }
         lastUsedIterator = currentIterator;
-
+        cachedHasNextValue = null;
         return currentIterator.next();
     }
 
@@ -241,10 +249,11 @@ public E next() {
     @Override
     public void remove() {
         lockChain();
-        if (currentIterator == null) {
-            updateCurrentIterator();
+        if (lastUsedIterator == null)  {
+            throw new IllegalStateException("remove() has been invoked without next()");
         }
         lastUsedIterator.remove();
+        lastUsedIterator = null;  // must never be used twice without next() being invoked
     }
 
     /**
@@ -267,13 +276,16 @@ protected void updateCurrentIterator() {
             } else {
                 currentIterator = iteratorQueue.remove();
             }
-            // set last used iterator here, in case the user calls remove
-            // before calling hasNext() or next() (although they shouldn't)
-            lastUsedIterator = currentIterator;
         }
-        while (!currentIterator.hasNext() && !iteratorQueue.isEmpty()) {
+        while (true) {
+            cachedHasNextValue = currentIterator.hasNext();
+            if (cachedHasNextValue) {
+                break;
+            }
+            if (iteratorQueue.isEmpty()) {
+                break;
+            }
             currentIterator = iteratorQueue.remove();
         }
     }
-
 }

src/test/java/org/apache/commons/collections4/SetUtilsTest.java

@@ -23,17 +23,22 @@
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.time.Duration;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.Set;
 
 import org.apache.commons.collections4.SetUtils.SetView;
 import org.apache.commons.collections4.set.PredicatedSet;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 /**
  * Tests for SetUtils.
@@ -218,6 +223,31 @@ void testUnion() {
         assertThrows(NullPointerException.class, () -> SetUtils.union(null, setA));
     }
 
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    void testReverseNestedUnionPerfomWell(final boolean mergeLeft) {
+        Set<Integer> set = SetUtils.union(setA, setB);
+        for (int i = 0; i < 128; i++) {
+            if (mergeLeft) {
+                set = SetUtils.union(setB, set);
+            } else {
+                set = SetUtils.union(set, setB);
+            }
+        }
+        final Set<Integer> combinedSet = set;
+        assertTimeoutPreemptively(Duration.ofSeconds(1), () -> {
+                assertEquals(7, combinedSet.size());
+                assertTrue(combinedSet.containsAll(setA));
+                assertTrue(combinedSet.containsAll(setB));
+
+                final Iterator<Integer> iterator = combinedSet.iterator();
+                while (iterator.hasNext()) { // without the IteratorChain hasNext() caching, this would run hours
+                    iterator.next();
+                }
+                assertFalse(iterator.hasNext());
+            });
+    }
+
     @Test
     void testUnmodifiableSet() {
         final Set<?> set1 = SetUtils.unmodifiableSet();

src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java

@@ -19,9 +19,12 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.time.Duration;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Iterator;
 import java.util.List;
 import java.util.NoSuchElementException;
@@ -126,8 +129,10 @@ void testFirstIteratorIsEmptyBug() {
         assertTrue(chain.hasNext(), "should have next");
         assertEquals("B", chain.next());
         assertTrue(chain.hasNext(), "should have next");
+        assertTrue(chain.hasNext(), "should not change");
         assertEquals("C", chain.next());
         assertFalse(chain.hasNext(), "should not have next");
+        assertFalse(chain.hasNext(), "should not change");
     }
 
     @Test
@@ -146,6 +151,9 @@ void testIterator() {
     public void testRemove() {
         final Iterator<String> iter = makeObject();
         assertThrows(IllegalStateException.class, () -> iter.remove(), "Calling remove before the first call to next() should throw an exception");
+        assertTrue(iter.hasNext(), "initial has next should be true");
+        assertThrows(IllegalStateException.class, () -> iter.remove(), "Calling remove before the first call to next() should throw an exception");
+
         for (final String testValue : testArray) {
             final String iterValue = iter.next();
             assertEquals(testValue, iterValue, "Iteration value is correct");
@@ -158,6 +166,78 @@ public void testRemove() {
         assertTrue(list3.isEmpty(), "List is empty");
     }
 
+    @Test
+    public void testRemoveDoubleCallShouldFail() {
+        final Iterator<String> iter = makeObject();
+        assertEquals(iter.next(), "One");
+        iter.remove();
+        assertThrows(IllegalStateException.class, () -> iter.remove());
+    }
+
+    @Test
+    public void testHasNextIsInvokedOnEdgeBeforeRemove() {
+        final Iterator<String> iter = makeObject();
+        assertEquals(iter.next(), "One");
+        assertEquals(iter.next(), "Two");
+        assertEquals(iter.next(), "Three");
+        assertTrue(iter.hasNext(), "next elements exists");
+        iter.remove();  // though hasNext() on next iterator has been invoked, removing an element on old iterator must still work
+        assertTrue(iter.hasNext(), "next elements exists");
+        assertEquals(iter.next(), "Four");
+
+        assertEquals(list1, Arrays.asList("One", "Two")); // Three must be gone
+        assertEquals(list2, Arrays.asList("Four")); // Four still be there
+        assertEquals(list3, Arrays.asList("Five", "Six")); // Five+Six anyway
+    }
+
+    @Test
+    public void testChainingPerformsWell() {
+        Iterator<String> iter = makeObject();
+        for (int i = 0; i < 150; i++) {
+            final IteratorChain<String> chain = new IteratorChain<>();
+            chain.addIterator(iter);
+            iter = chain;
+        }
+        final Iterator<String> iterFinal = iter;
+        assertTimeoutPreemptively(Duration.ofSeconds(1), () -> {
+                for (final String testValue : testArray) {
+                    final String iterValue = iterFinal.next();
+                    assertEquals(testValue, iterValue, "Iteration value is correct");
+                    if (!iterValue.equals("Four")) {
+                        iterFinal.remove();
+                    }
+                }
+                assertFalse(iterFinal.hasNext(), "all values got iterated");
+                assertTrue(list1.isEmpty(), "List is empty");
+                assertEquals(1, list2.size(), "List is empty");
+                assertTrue(list3.isEmpty(), "List is empty");
+            });
+    }
+
+    @Test
+    public void testChaining() {
+        IteratorChain<String> chain = new IteratorChain<>();
+        chain.addIterator(list1.iterator());
+        chain = new IteratorChain<>(chain);
+        chain.addIterator(list2.iterator());
+        chain = new IteratorChain<>(chain);
+        chain.addIterator(list3.iterator());
+
+        for (final String testValue : testArray) {
+            assertTrue(chain.hasNext(), "chain contains values");
+            assertTrue(chain.hasNext(), "hasNext doesn't change on 2nd invocation");
+            final String iterValue = chain.next();
+            assertEquals(testValue, iterValue, "Iteration value is correct");
+            if (!iterValue.equals("Four")) {
+                chain.remove();
+            }
+        }
+        assertFalse(chain.hasNext(), "all values got iterated");
+        assertTrue(list1.isEmpty(), "List is empty");
+        assertEquals(1, list2.size(), "List is empty");
+        assertTrue(list3.isEmpty(), "List is empty");
+    }
+
     @Test
     void testRemoveFromFilteredIterator() {