diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 1f59bf07eb..e90b8015ed 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -32,6 +32,7 @@ MapUtils.getLongValue(Map, K, Function) returns a byte instead of a long. Fix exception message in org.apache.commons.collections4.functors.FunctorUtils.validate(Consumer...) Fix exception message in org.apache.commons.collections4.iterators.UnmodifiableIterator.remove() to match java.util.Iterator.remove(). + Calling SetUtils.union on multiple instances of SetView causes JVM to hang Add generics to UnmodifiableIterator for the wrapped type. diff --git a/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java b/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java index 7c69f3f749..97373b13cd 100644 --- a/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java +++ b/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java @@ -63,7 +63,7 @@ public class IteratorChain implements Iterator { private Iterator 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 lastUsedIterator; @@ -74,6 +74,11 @@ public class IteratorChain implements Iterator { */ private boolean isLocked; + /** + * Contains the result of the last hasNext() call until next() is invoked + */ + private Boolean cachedHasNextValue; + /** * Constructs an IteratorChain with no Iterators. *

@@ -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(); } } - } diff --git a/src/test/java/org/apache/commons/collections4/SetUtilsTest.java b/src/test/java/org/apache/commons/collections4/SetUtilsTest.java index ffb7bc3a18..27a0965f78 100644 --- a/src/test/java/org/apache/commons/collections4/SetUtilsTest.java +++ b/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 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 combinedSet = set; + assertTimeoutPreemptively(Duration.ofSeconds(1), () -> { + assertEquals(7, combinedSet.size()); + assertTrue(combinedSet.containsAll(setA)); + assertTrue(combinedSet.containsAll(setB)); + + final Iterator 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(); diff --git a/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java b/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java index a452118533..f1cff577f2 100644 --- a/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java +++ b/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 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 iter = makeObject(); + assertEquals(iter.next(), "One"); + iter.remove(); + assertThrows(IllegalStateException.class, () -> iter.remove()); + } + + @Test + public void testHasNextIsInvokedOnEdgeBeforeRemove() { + final Iterator 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 iter = makeObject(); + for (int i = 0; i < 150; i++) { + final IteratorChain chain = new IteratorChain<>(); + chain.addIterator(iter); + iter = chain; + } + final Iterator 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 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() {