From ca80d52051e37851f9d62570bf7bb6cfbb5eb047 Mon Sep 17 00:00:00 2001 From: sankalp suman Date: Tue, 23 Jun 2026 11:00:09 +0530 Subject: [PATCH 1/6] COLLECTIONS-776: Wrapping PassiveExpiringMap in a SynchronizedMap breaks expiration --- src/changes/changes.xml | 1 + .../collections4/map/PassiveExpiringMap.java | 436 +++++++++++++++++- .../map/PassiveExpiringMapTest.java | 66 +++ 3 files changed, 500 insertions(+), 3 deletions(-) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 2bc865a4fc..d2f86f4513 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -24,6 +24,7 @@ + Wrapping PassiveExpiringMap in a SynchonizedMap breaks expiration. org.apache.commons.collections4.map.AbstractReferenceMap.ReferenceEntry.toReference(ReferenceStrength, T, int) now throws IllegalArgumentException instead of Error. Remove deprecation annotation of org.apache.commons.collections4.Factory; this will be deprecated in 5.0 in favor of java.util.function.Supplier. Remove deprecation annotation of org.apache.commons.collections4.Predicate; this will be deprecated in 5.0 in favor of java.util.function.Predicate. diff --git a/src/main/java/org/apache/commons/collections4/map/PassiveExpiringMap.java b/src/main/java/org/apache/commons/collections4/map/PassiveExpiringMap.java index d435645736..59e38f7c07 100644 --- a/src/main/java/org/apache/commons/collections4/map/PassiveExpiringMap.java +++ b/src/main/java/org/apache/commons/collections4/map/PassiveExpiringMap.java @@ -27,6 +27,11 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.TimeUnit; +import java.util.function.Predicate; + +import org.apache.commons.collections4.collection.AbstractCollectionDecorator; +import org.apache.commons.collections4.iterators.AbstractIteratorDecorator; +import org.apache.commons.collections4.set.AbstractSetDecorator; /** * Decorates a {@code Map} to evict expired entries once their expiration @@ -359,7 +364,7 @@ public boolean containsValue(final Object value) { @Override public Set> entrySet() { removeAllExpired(now()); - return super.entrySet(); + return new EntrySet(super.entrySet()); } /** @@ -408,7 +413,7 @@ private boolean isExpired(final long now, final Long expirationTimeObject) { @Override public Set keySet() { removeAllExpired(now()); - return super.keySet(); + return new KeySet(super.keySet()); } /** @@ -519,7 +524,7 @@ public int size() { @Override public Collection values() { removeAllExpired(now()); - return super.values(); + return new ValuesCollection(super.values()); } /** @@ -533,4 +538,429 @@ private void writeObject(final ObjectOutputStream out) out.defaultWriteObject(); out.writeObject(map); } + + private final class EntrySet extends AbstractSetDecorator> { + + /** Generated serial version ID. */ + private static final long serialVersionUID = 1L; + + private EntrySet(final Set> set) { + super(set); + } + + @Override + public Iterator> iterator() { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return new EntrySetIterator(super.iterator()); + } + + @Override + public int size() { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.size(); + } + + @Override + public boolean isEmpty() { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.isEmpty(); + } + + @Override + public boolean contains(final Object object) { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.contains(object); + } + + @Override + public boolean remove(final Object object) { + if (object instanceof Map.Entry) { + final Map.Entry entry = (Map.Entry) object; + final Object key = entry.getKey(); + if (PassiveExpiringMap.this.containsKey(key)) { + final Object value = PassiveExpiringMap.this.get(key); + if (Objects.equals(value, entry.getValue())) { + PassiveExpiringMap.this.remove(key); + return true; + } + } + } + return false; + } + + @Override + public boolean removeAll(final Collection coll) { + if (coll == null) { + return false; + } + boolean changed = false; + if (size() > coll.size()) { + for (final Object obj : coll) { + changed |= remove(obj); + } + } else { + final Iterator it = iterator(); + while (it.hasNext()) { + if (coll.contains(it.next())) { + it.remove(); + changed = true; + } + } + } + return changed; + } + + @Override + public boolean retainAll(final Collection coll) { + if (coll == null) { + return false; + } + boolean changed = false; + final Iterator it = iterator(); + while (it.hasNext()) { + if (!coll.contains(it.next())) { + it.remove(); + changed = true; + } + } + return changed; + } + + @Override + public boolean removeIf(final Predicate> filter) { + Objects.requireNonNull(filter); + boolean changed = false; + final Iterator> it = iterator(); + while (it.hasNext()) { + if (filter.test(it.next())) { + it.remove(); + changed = true; + } + } + return changed; + } + + @Override + public void clear() { + PassiveExpiringMap.this.clear(); + } + + @Override + public Object[] toArray() { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.toArray(); + } + + @Override + public T[] toArray(final T[] array) { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.toArray(array); + } + + @Override + public boolean containsAll(final Collection coll) { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.containsAll(coll); + } + } + + private final class EntrySetIterator extends AbstractIteratorDecorator> { + private Entry lastReturned; + + private EntrySetIterator(final Iterator> iterator) { + super(iterator); + } + + @Override + public Entry next() { + lastReturned = super.next(); + return lastReturned; + } + + @Override + public void remove() { + super.remove(); + if (lastReturned != null) { + PassiveExpiringMap.this.expirationMap.remove(lastReturned.getKey()); + lastReturned = null; + } + } + } + + private final class KeySet extends AbstractSetDecorator { + + /** Generated serial version ID. */ + private static final long serialVersionUID = 1L; + + private KeySet(final Set set) { + super(set); + } + + @Override + public Iterator iterator() { + return new KeySetIterator(PassiveExpiringMap.this.entrySet().iterator()); + } + + @Override + public int size() { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.size(); + } + + @Override + public boolean isEmpty() { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.isEmpty(); + } + + @Override + public boolean contains(final Object key) { + PassiveExpiringMap.this.removeIfExpired(key, PassiveExpiringMap.this.now()); + return super.contains(key); + } + + @Override + public boolean remove(final Object key) { + final boolean hasKey = contains(key); + if (hasKey) { + PassiveExpiringMap.this.remove(key); + } + return hasKey; + } + + @Override + public boolean removeAll(final Collection coll) { + if (coll == null) { + return false; + } + boolean changed = false; + if (size() > coll.size()) { + for (final Object obj : coll) { + changed |= remove(obj); + } + } else { + final Iterator it = iterator(); + while (it.hasNext()) { + if (coll.contains(it.next())) { + it.remove(); + changed = true; + } + } + } + return changed; + } + + @Override + public boolean retainAll(final Collection coll) { + if (coll == null) { + return false; + } + boolean changed = false; + final Iterator it = iterator(); + while (it.hasNext()) { + if (!coll.contains(it.next())) { + it.remove(); + changed = true; + } + } + return changed; + } + + @Override + public boolean removeIf(final Predicate filter) { + Objects.requireNonNull(filter); + boolean changed = false; + final Iterator it = iterator(); + while (it.hasNext()) { + if (filter.test(it.next())) { + it.remove(); + changed = true; + } + } + return changed; + } + + @Override + public void clear() { + PassiveExpiringMap.this.clear(); + } + + @Override + public Object[] toArray() { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.toArray(); + } + + @Override + public T[] toArray(final T[] array) { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.toArray(array); + } + + @Override + public boolean containsAll(final Collection coll) { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.containsAll(coll); + } + } + + private final class KeySetIterator implements Iterator { + private final Iterator> iterator; + + private KeySetIterator(final Iterator> iterator) { + this.iterator = iterator; + } + + @Override + public boolean hasNext() { + return iterator.hasNext(); + } + + @Override + public K next() { + return iterator.next().getKey(); + } + + @Override + public void remove() { + iterator.remove(); + } + } + + private final class ValuesCollection extends AbstractCollectionDecorator { + + /** Generated serial version ID. */ + private static final long serialVersionUID = 1L; + + private ValuesCollection(final Collection coll) { + super(coll); + } + + @Override + public Iterator iterator() { + return new ValuesIterator(PassiveExpiringMap.this.entrySet().iterator()); + } + + @Override + public int size() { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.size(); + } + + @Override + public boolean isEmpty() { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.isEmpty(); + } + + @Override + public boolean contains(final Object value) { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.contains(value); + } + + @Override + public boolean remove(final Object value) { + final Iterator it = iterator(); + while (it.hasNext()) { + if (Objects.equals(it.next(), value)) { + it.remove(); + return true; + } + } + return false; + } + + @Override + public boolean removeAll(final Collection coll) { + if (coll == null) { + return false; + } + boolean changed = false; + final Iterator it = iterator(); + while (it.hasNext()) { + if (coll.contains(it.next())) { + it.remove(); + changed = true; + } + } + return changed; + } + + @Override + public boolean retainAll(final Collection coll) { + if (coll == null) { + return false; + } + boolean changed = false; + final Iterator it = iterator(); + while (it.hasNext()) { + if (!coll.contains(it.next())) { + it.remove(); + changed = true; + } + } + return changed; + } + + @Override + public boolean removeIf(final Predicate filter) { + Objects.requireNonNull(filter); + boolean changed = false; + final Iterator it = iterator(); + while (it.hasNext()) { + if (filter.test(it.next())) { + it.remove(); + changed = true; + } + } + return changed; + } + + @Override + public void clear() { + PassiveExpiringMap.this.clear(); + } + + @Override + public Object[] toArray() { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.toArray(); + } + + @Override + public T[] toArray(final T[] array) { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.toArray(array); + } + + @Override + public boolean containsAll(final Collection coll) { + PassiveExpiringMap.this.removeAllExpired(PassiveExpiringMap.this.now()); + return super.containsAll(coll); + } + } + + private final class ValuesIterator implements Iterator { + private final Iterator> iterator; + + private ValuesIterator(final Iterator> iterator) { + this.iterator = iterator; + } + + @Override + public boolean hasNext() { + return iterator.hasNext(); + } + + @Override + public V next() { + return iterator.next().getValue(); + } + + @Override + public void remove() { + iterator.remove(); + } + } } diff --git a/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java b/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java index 3d179a27b8..7720cd178b 100644 --- a/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java @@ -23,7 +23,9 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.util.Collections; import java.util.HashMap; +import java.util.Iterator; import java.util.Map; import java.util.concurrent.TimeUnit; @@ -255,4 +257,68 @@ private void validateExpiration(final Map map, final long timeou assertNull(map.get("a")); } + @Test + void testCollectionsSynchronizedMapExpiration() throws InterruptedException { + final Map map = Collections.synchronizedMap(new PassiveExpiringMap<>(50L)); + map.put("a", "b"); + map.put("c", "d"); + assertEquals(2, map.size()); + Thread.sleep(100L); + + // entrySet iterator triggers expiration + synchronized (map) { + assertEquals(0, map.entrySet().size()); + } + + map.put("a", "b"); + map.put("c", "d"); + assertEquals(2, map.size()); + Thread.sleep(100L); + + // keySet iterator triggers expiration + synchronized (map) { + assertEquals(0, map.keySet().size()); + } + + map.put("a", "b"); + map.put("c", "d"); + assertEquals(2, map.size()); + Thread.sleep(100L); + + // values iterator triggers expiration + synchronized (map) { + assertEquals(0, map.values().size()); + } + } + + @Test + void testCollectionViewRemoval() { + final PassiveExpiringMap map = new PassiveExpiringMap<>(10000L); + map.put("a", "b"); + map.put("c", "d"); + map.put("e", "f"); + + // Remove via entrySet iterator + final Iterator> entryIter = map.entrySet().iterator(); + assertTrue(entryIter.hasNext()); + final Map.Entry entry = entryIter.next(); + final String removedKey = entry.getKey(); + entryIter.remove(); + assertFalse(map.containsKey(removedKey)); + + // Remove via keySet iterator + final Iterator keyIter = map.keySet().iterator(); + assertTrue(keyIter.hasNext()); + final String key = keyIter.next(); + keyIter.remove(); + assertFalse(map.containsKey(key)); + + // Remove via values iterator + final Iterator valIter = map.values().iterator(); + assertTrue(valIter.hasNext()); + final String val = valIter.next(); + valIter.remove(); + assertFalse(map.containsValue(val)); + } + } From 02cfcd8555d79cbf461ad7fe884b81a51cb0c3dc Mon Sep 17 00:00:00 2001 From: sankalp suman Date: Wed, 24 Jun 2026 10:49:18 +0530 Subject: [PATCH 2/6] COLLECTIONS-776: Update regression test to cache views before expiration to satisfy TDD --- .../collections4/map/PassiveExpiringMapTest.java | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java b/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java index 7720cd178b..bd4600bdb2 100644 --- a/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java @@ -23,10 +23,12 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.Map; +import java.util.Set; import java.util.concurrent.TimeUnit; import org.apache.commons.collections4.collection.AbstractCollectionTest; @@ -263,11 +265,17 @@ void testCollectionsSynchronizedMapExpiration() throws InterruptedException { map.put("a", "b"); map.put("c", "d"); assertEquals(2, map.size()); + + // Cache the views in SynchronizedMap before they expire + final Collection> entrySet = map.entrySet(); + final Collection keySet = map.keySet(); + final Collection values = map.values(); + Thread.sleep(100L); // entrySet iterator triggers expiration synchronized (map) { - assertEquals(0, map.entrySet().size()); + assertEquals(0, entrySet.size()); } map.put("a", "b"); @@ -277,7 +285,7 @@ void testCollectionsSynchronizedMapExpiration() throws InterruptedException { // keySet iterator triggers expiration synchronized (map) { - assertEquals(0, map.keySet().size()); + assertEquals(0, keySet.size()); } map.put("a", "b"); @@ -287,7 +295,7 @@ void testCollectionsSynchronizedMapExpiration() throws InterruptedException { // values iterator triggers expiration synchronized (map) { - assertEquals(0, map.values().size()); + assertEquals(0, values.size()); } } From c22a6e65ec20f6f50f64a8f41bafe8c2a140b2f1 Mon Sep 17 00:00:00 2001 From: Gary Gregory Date: Sat, 27 Jun 2026 14:59:08 -0400 Subject: [PATCH 3/6] Remove unused. --- .../apache/commons/collections4/map/PassiveExpiringMapTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java b/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java index bd4600bdb2..253f8ff5a0 100644 --- a/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java @@ -28,7 +28,6 @@ import java.util.HashMap; import java.util.Iterator; import java.util.Map; -import java.util.Set; import java.util.concurrent.TimeUnit; import org.apache.commons.collections4.collection.AbstractCollectionTest; From 2769f27611b85531680566227022c7c160444f62 Mon Sep 17 00:00:00 2001 From: sankalp suman Date: Sun, 28 Jun 2026 01:29:03 +0530 Subject: [PATCH 4/6] COLLECTIONS-776: Fix tests to use isEmpty(), clean blank lines, and cover null removeAll/retainAll --- .../map/PassiveExpiringMapTest.java | 31 +++++++++++-------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java b/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java index 253f8ff5a0..42d60fd0a1 100644 --- a/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java @@ -264,37 +264,30 @@ void testCollectionsSynchronizedMapExpiration() throws InterruptedException { map.put("a", "b"); map.put("c", "d"); assertEquals(2, map.size()); - // Cache the views in SynchronizedMap before they expire final Collection> entrySet = map.entrySet(); final Collection keySet = map.keySet(); final Collection values = map.values(); - Thread.sleep(100L); - // entrySet iterator triggers expiration synchronized (map) { - assertEquals(0, entrySet.size()); + assertTrue(entrySet.isEmpty()); } - map.put("a", "b"); map.put("c", "d"); assertEquals(2, map.size()); Thread.sleep(100L); - // keySet iterator triggers expiration synchronized (map) { - assertEquals(0, keySet.size()); + assertTrue(keySet.isEmpty()); } - map.put("a", "b"); map.put("c", "d"); assertEquals(2, map.size()); Thread.sleep(100L); - // values iterator triggers expiration synchronized (map) { - assertEquals(0, values.size()); + assertTrue(values.isEmpty()); } } @@ -304,7 +297,6 @@ void testCollectionViewRemoval() { map.put("a", "b"); map.put("c", "d"); map.put("e", "f"); - // Remove via entrySet iterator final Iterator> entryIter = map.entrySet().iterator(); assertTrue(entryIter.hasNext()); @@ -312,14 +304,12 @@ void testCollectionViewRemoval() { final String removedKey = entry.getKey(); entryIter.remove(); assertFalse(map.containsKey(removedKey)); - // Remove via keySet iterator final Iterator keyIter = map.keySet().iterator(); assertTrue(keyIter.hasNext()); final String key = keyIter.next(); keyIter.remove(); assertFalse(map.containsKey(key)); - // Remove via values iterator final Iterator valIter = map.values().iterator(); assertTrue(valIter.hasNext()); @@ -328,4 +318,19 @@ void testCollectionViewRemoval() { assertFalse(map.containsValue(val)); } + @Test + void testCollectionViewNullInputs() { + final PassiveExpiringMap map = new PassiveExpiringMap<>(10000L); + map.put("a", "b"); + // entrySet + assertFalse(map.entrySet().removeAll(null)); + assertFalse(map.entrySet().retainAll(null)); + // keySet + assertFalse(map.keySet().removeAll(null)); + assertFalse(map.keySet().retainAll(null)); + // values + assertFalse(map.values().removeAll(null)); + assertFalse(map.values().retainAll(null)); + } + } From 6f604105812e4e7977bfbfa5a2d6afeeb00f3eaa Mon Sep 17 00:00:00 2001 From: Gary Gregory Date: Sun, 28 Jun 2026 10:44:21 -0400 Subject: [PATCH 5/6] Better test assertions. --- .../commons/collections4/map/PassiveExpiringMapTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java b/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java index 42d60fd0a1..6a911ab448 100644 --- a/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java @@ -272,6 +272,8 @@ void testCollectionsSynchronizedMapExpiration() throws InterruptedException { // entrySet iterator triggers expiration synchronized (map) { assertTrue(entrySet.isEmpty()); + assertTrue(keySet.isEmpty()); + assertTrue(values.isEmpty()); } map.put("a", "b"); map.put("c", "d"); @@ -279,7 +281,9 @@ void testCollectionsSynchronizedMapExpiration() throws InterruptedException { Thread.sleep(100L); // keySet iterator triggers expiration synchronized (map) { + assertTrue(entrySet.isEmpty()); assertTrue(keySet.isEmpty()); + assertTrue(values.isEmpty()); } map.put("a", "b"); map.put("c", "d"); @@ -287,6 +291,8 @@ void testCollectionsSynchronizedMapExpiration() throws InterruptedException { Thread.sleep(100L); // values iterator triggers expiration synchronized (map) { + assertTrue(entrySet.isEmpty()); + assertTrue(keySet.isEmpty()); assertTrue(values.isEmpty()); } } From 95cb99361c00fa7e86101a2dc7b0bb1fb6320ae9 Mon Sep 17 00:00:00 2001 From: sankalp suman Date: Sun, 28 Jun 2026 20:48:53 +0530 Subject: [PATCH 6/6] COLLECTIONS-776: Add test case for cached view iterator expiration --- .../map/PassiveExpiringMapTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java b/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java index 6a911ab448..87a33c128c 100644 --- a/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java +++ b/src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java @@ -339,4 +339,21 @@ void testCollectionViewNullInputs() { assertFalse(map.values().retainAll(null)); } + @Test + void testCollectionViewIteratorExpiration() throws InterruptedException { + final PassiveExpiringMap map = new PassiveExpiringMap<>(50L); + map.put("a", "b"); + + final Collection> entrySet = map.entrySet(); + final Collection keySet = map.keySet(); + final Collection values = map.values(); + + Thread.sleep(100L); + + // The iterators should trigger expiration and not return any elements + assertFalse(entrySet.iterator().hasNext()); + assertFalse(keySet.iterator().hasNext()); + assertFalse(values.iterator().hasNext()); + } + }