From 7ad905fa0b3757b6f7390ed82c27f50551716583 Mon Sep 17 00:00:00 2001 From: Joerg Budischewski Date: Sun, 10 Aug 2025 00:17:13 +0200 Subject: [PATCH 1/7] [collections-838] hasNext() value is now cached till next() or remove() get invoked, thus reducing call load on hasNext() dratically especially in nested chained iterator scenarios --- .../collections4/iterators/IteratorChain.java | 17 +++++-- .../iterators/IteratorChainTest.java | 51 +++++++++++++++++++ 2 files changed, 64 insertions(+), 4 deletions(-) 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..9ba4c933fb 100644 --- a/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java +++ b/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java @@ -74,6 +74,11 @@ public class IteratorChain implements Iterator { */ private boolean isLocked; + /** + * Contains the result of the last hasNext() call until either next() or remove() is invoked + */ + private Boolean cachedHasNextValue; + /** * Constructs an IteratorChain with no Iterators. *

@@ -183,9 +188,12 @@ private void checkLocked() { @Override public boolean hasNext() { lockChain(); - updateCurrentIterator(); - lastUsedIterator = currentIterator; - return currentIterator.hasNext(); + if (cachedHasNextValue == null) { + updateCurrentIterator(); + lastUsedIterator = currentIterator; + cachedHasNextValue = currentIterator.hasNext(); + } + return cachedHasNextValue; } /** @@ -221,7 +229,7 @@ public E next() { lockChain(); updateCurrentIterator(); lastUsedIterator = currentIterator; - + cachedHasNextValue = null; return currentIterator.next(); } @@ -244,6 +252,7 @@ public void remove() { if (currentIterator == null) { updateCurrentIterator(); } + cachedHasNextValue = null; lastUsedIterator.remove(); } 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..c4b81bb900 100644 --- a/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java +++ b/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java @@ -126,8 +126,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 +148,30 @@ 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"); + if (!iterValue.equals("Four")) { + iter.remove(); + } + } + assertTrue(list1.isEmpty(), "List is empty"); + assertEquals(1, list2.size(), "List is empty"); + assertTrue(list3.isEmpty(), "List is empty"); + } + + @Test + public void testChainingPerformsWell() { + Iterator iter = makeObject(); + for (int i = 0; i < 150; i++) { + final IteratorChain chain = new IteratorChain<>(); + chain.addIterator(iter); + iter = chain; + } + for (final String testValue : testArray) { final String iterValue = iter.next(); assertEquals(testValue, iterValue, "Iteration value is correct"); @@ -153,6 +179,31 @@ public void testRemove() { iter.remove(); } } + assertFalse(iter.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"); From ac8f46128cadd61874108a2a4572a0a3323049a7 Mon Sep 17 00:00:00 2001 From: Joerg Budischewski Date: Sun, 10 Aug 2025 00:23:34 +0200 Subject: [PATCH 2/7] [collections-838] fixed one slipped through checkstyle violation --- .../commons/collections4/iterators/IteratorChainTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 c4b81bb900..a0693fa962 100644 --- a/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java +++ b/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java @@ -148,7 +148,7 @@ 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"); + 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) { From da78928adffd1208cbcb08445406bbf022eed728 Mon Sep 17 00:00:00 2001 From: Joerg Budischewski Date: Sun, 10 Aug 2025 01:27:40 +0200 Subject: [PATCH 3/7] [collections-838] added test for SetUtils.union(), now using assertTimeoutPreemptively() for tests --- .../commons/collections4/SetUtilsTest.java | 30 +++++++++++++++++++ .../iterators/IteratorChainTest.java | 28 +++++++++-------- 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/src/test/java/org/apache/commons/collections4/SetUtilsTest.java b/src/test/java/org/apache/commons/collections4/SetUtilsTest.java index ffb7bc3a18..51c2e856c5 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 < 40; 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 a0693fa962..a1c71d0bc4 100644 --- a/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java +++ b/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java @@ -19,8 +19,10 @@ 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.Iterator; import java.util.List; @@ -171,18 +173,20 @@ public void testChainingPerformsWell() { chain.addIterator(iter); iter = chain; } - - for (final String testValue : testArray) { - final String iterValue = iter.next(); - assertEquals(testValue, iterValue, "Iteration value is correct"); - if (!iterValue.equals("Four")) { - iter.remove(); - } - } - assertFalse(iter.hasNext(), "all values got iterated"); - assertTrue(list1.isEmpty(), "List is empty"); - assertEquals(1, list2.size(), "List is empty"); - assertTrue(list3.isEmpty(), "List is empty"); + 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 From 6c2c60415ce9b0bc844643ce7417850e7d164d75 Mon Sep 17 00:00:00 2001 From: Joerg Budischewski Date: Tue, 12 Aug 2025 22:00:16 +0200 Subject: [PATCH 4/7] [collections-838] optimize next() to not recalculate currentIterator when hasNext() has been invoked already --- .../apache/commons/collections4/iterators/IteratorChain.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 9ba4c933fb..8199d08e9d 100644 --- a/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java +++ b/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java @@ -227,7 +227,9 @@ private void lockChain() { @Override public E next() { lockChain(); - updateCurrentIterator(); + if( cachedHasNextValue == null ) { + updateCurrentIterator(); + } lastUsedIterator = currentIterator; cachedHasNextValue = null; return currentIterator.next(); From f9b9c04e3a14ed20b81f583380d2368c99e40457 Mon Sep 17 00:00:00 2001 From: Joerg Budischewski Date: Tue, 12 Aug 2025 23:16:44 +0200 Subject: [PATCH 5/7] [collections-838] fixed remove-may-fail-when-hasNext-has-been-invoked-before bug, further optimization to save hasNext() calls --- .../collections4/iterators/IteratorChain.java | 23 +++++++++-------- .../iterators/IteratorChainTest.java | 25 +++++++++++++++++++ 2 files changed, 37 insertions(+), 11 deletions(-) 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 8199d08e9d..9ad8b386c8 100644 --- a/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java +++ b/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java @@ -190,8 +190,6 @@ public boolean hasNext() { lockChain(); if (cachedHasNextValue == null) { updateCurrentIterator(); - lastUsedIterator = currentIterator; - cachedHasNextValue = currentIterator.hasNext(); } return cachedHasNextValue; } @@ -227,7 +225,7 @@ private void lockChain() { @Override public E next() { lockChain(); - if( cachedHasNextValue == null ) { + if (cachedHasNextValue == null) { updateCurrentIterator(); } lastUsedIterator = currentIterator; @@ -251,11 +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()"); } - cachedHasNextValue = null; lastUsedIterator.remove(); + lastUsedIterator = null; // must never be used twice without next() being invoked } /** @@ -278,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/iterators/IteratorChainTest.java b/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java index a1c71d0bc4..b3a2f52cc2 100644 --- a/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java +++ b/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java @@ -24,6 +24,7 @@ import java.time.Duration; import java.util.ArrayList; +import java.util.Arrays; import java.util.Iterator; import java.util.List; import java.util.NoSuchElementException; @@ -165,6 +166,30 @@ 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, remove on 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")); // Four still be there + } + @Test public void testChainingPerformsWell() { Iterator iter = makeObject(); From 342221e1e6f8cbaabed5604e00f1bb276b7f2e6b Mon Sep 17 00:00:00 2001 From: Joerg Budischewski Date: Tue, 12 Aug 2025 23:26:51 +0200 Subject: [PATCH 6/7] [collections-838] fixing minor doc typos --- .../apache/commons/collections4/iterators/IteratorChain.java | 4 ++-- .../commons/collections4/iterators/IteratorChainTest.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) 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 9ad8b386c8..a7e4dd5e84 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; @@ -75,7 +75,7 @@ public class IteratorChain implements Iterator { private boolean isLocked; /** - * Contains the result of the last hasNext() call until either next() or remove() is invoked + * Contains the result of the last hasNext() call until next() is invoked */ private Boolean cachedHasNextValue; 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 b3a2f52cc2..f1cff577f2 100644 --- a/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java +++ b/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java @@ -181,13 +181,13 @@ public void testHasNextIsInvokedOnEdgeBeforeRemove() { assertEquals(iter.next(), "Two"); assertEquals(iter.next(), "Three"); assertTrue(iter.hasNext(), "next elements exists"); - iter.remove(); // though hasNext() on next iterator has been invoked, remove on element on old iterator must still work + 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")); // Four still be there + assertEquals(list3, Arrays.asList("Five", "Six")); // Five+Six anyway } @Test From 30d07fe7f8ed7514673694328bb386415cb28215 Mon Sep 17 00:00:00 2001 From: Joerg Budischewski Date: Wed, 13 Aug 2025 21:22:04 +0200 Subject: [PATCH 7/7] [collections-838] fixing review remarks, typo fix, adding line to changes.xml --- src/changes/changes.xml | 1 + .../apache/commons/collections4/iterators/IteratorChain.java | 2 +- src/test/java/org/apache/commons/collections4/SetUtilsTest.java | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) 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 a7e4dd5e84..97373b13cd 100644 --- a/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java +++ b/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java @@ -250,7 +250,7 @@ public E next() { public void remove() { lockChain(); if (lastUsedIterator == null) { - throw new IllegalStateException("remove has been invoked() without next()"); + throw new IllegalStateException("remove() has been invoked without next()"); } lastUsedIterator.remove(); lastUsedIterator = null; // must never be used twice without next() being invoked diff --git a/src/test/java/org/apache/commons/collections4/SetUtilsTest.java b/src/test/java/org/apache/commons/collections4/SetUtilsTest.java index 51c2e856c5..27a0965f78 100644 --- a/src/test/java/org/apache/commons/collections4/SetUtilsTest.java +++ b/src/test/java/org/apache/commons/collections4/SetUtilsTest.java @@ -227,7 +227,7 @@ void testUnion() { @ValueSource(booleans = {true, false}) void testReverseNestedUnionPerfomWell(final boolean mergeLeft) { Set set = SetUtils.union(setA, setB); - for (int i = 0; i < 40; i++) { + for (int i = 0; i < 128; i++) { if (mergeLeft) { set = SetUtils.union(setB, set); } else {