This is an automated email from the ASF dual-hosted git repository.
garydgregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-collections.git
The following commit(s) were added to refs/heads/master by this push:
new 491fc7758 [COLLECTIONS-776] Fix expiration bypass in
PassiveExpiringMap when wrapped in SynchronizedMap (#694)
491fc7758 is described below
commit 491fc77589f1b262d19f5c109fc42794edfcb36e
Author: sankalp suman <[email protected]>
AuthorDate: Mon Jul 6 01:20:01 2026 +0530
[COLLECTIONS-776] Fix expiration bypass in PassiveExpiringMap when wrapped
in SynchronizedMap (#694)
* COLLECTIONS-776: Wrapping PassiveExpiringMap in a SynchronizedMap breaks
expiration
* COLLECTIONS-776: Update regression test to cache views before expiration
to satisfy TDD
* Remove unused.
* COLLECTIONS-776: Fix tests to use isEmpty(), clean blank lines, and cover
null removeAll/retainAll
* Better test assertions.
* COLLECTIONS-776: Add test case for cached view iterator expiration
* COLLECTIONS-776: Align collection views removeAll/retainAll with JDK
semantics and test expirationMap cleanup
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI
<[email protected]>
* COLLECTIONS-776: Update comments in
testCollectionsSynchronizedMapExpiration to reflect view access triggers
expiration
---------
Co-authored-by: Gary Gregory <[email protected]>
Co-authored-by: Copilot Autofix powered by AI
<[email protected]>
---
src/changes/changes.xml | 1 +
.../collections4/map/PassiveExpiringMap.java | 424 ++++++++++++++++++++-
.../collections4/map/PassiveExpiringMapTest.java | 184 +++++++++
3 files changed, 606 insertions(+), 3 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 972914bbd..24535388a 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -24,6 +24,7 @@
<body>
<release version="4.6.0" date="YYYY-MM-DD" description="This is a feature
and maintenance release. Java 8 or later is required.">
<!-- FIX -->
+ <action type="fix" dev="ggregory" due-to="Gary Gregory"
issue="COLLECTIONS-776">Wrapping PassiveExpiringMap in
Collections.synchronizedMap breaks expiration.</action>
<action type="fix" dev="ggregory" due-to="Naveed Khan, Gary
Gregory">[javadoc] Document that BloomFilter merge operations are non-atomic
and a filter that throws during a merge should be considered invalid.</action>
<action type="fix" dev="ggregory" due-to="Gary
Gregory">org.apache.commons.collections4.map.AbstractReferenceMap.ReferenceEntry.toReference(ReferenceStrength,
T, int) now throws IllegalArgumentException instead of Error.</action>
<action type="fix" dev="ggregory" due-to="Eric Hubert, Gary
Gregory">Remove deprecation annotation of
org.apache.commons.collections4.Factory; this will be deprecated in 5.0 in
favor of java.util.function.Supplier.</action>
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 4de0ef9a3..eb1eb1c7f 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.Map;
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 class PassiveExpiringMap<K, V>
@Override
public Set<Entry<K, V>> entrySet() {
removeAllExpired(now());
- return super.entrySet();
+ return new EntrySet(super.entrySet());
}
/**
@@ -408,7 +413,7 @@ public class PassiveExpiringMap<K, V>
@Override
public Set<K> keySet() {
removeAllExpired(now());
- return super.keySet();
+ return new KeySet(super.keySet());
}
/**
@@ -519,7 +524,7 @@ public class PassiveExpiringMap<K, V>
@Override
public Collection<V> values() {
removeAllExpired(now());
- return super.values();
+ return new ValuesCollection(super.values());
}
/**
@@ -533,4 +538,417 @@ public class PassiveExpiringMap<K, V>
out.defaultWriteObject();
out.writeObject(map);
}
+
+ private final class EntrySet extends AbstractSetDecorator<Entry<K, V>> {
+
+ /** Generated serial version ID. */
+ private static final long serialVersionUID = 1L;
+
+ private EntrySet(final Set<Entry<K, V>> set) {
+ super(set);
+ }
+
+ @Override
+ public Iterator<Entry<K, V>> 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) {
+ Objects.requireNonNull(coll);
+ 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) {
+ Objects.requireNonNull(coll);
+ 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<? super Entry<K, V>> filter) {
+ Objects.requireNonNull(filter);
+ boolean changed = false;
+ final Iterator<Entry<K, V>> 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> 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<Entry<K, V>> {
+ private Entry<K, V> lastReturned;
+
+ private EntrySetIterator(final Iterator<Entry<K, V>> iterator) {
+ super(iterator);
+ }
+
+ @Override
+ public Entry<K, V> 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<K> {
+
+ /** Generated serial version ID. */
+ private static final long serialVersionUID = 1L;
+
+ private KeySet(final Set<K> set) {
+ super(set);
+ }
+
+ @Override
+ public Iterator<K> 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) {
+ Objects.requireNonNull(coll);
+ 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) {
+ Objects.requireNonNull(coll);
+ 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<? super K> filter) {
+ Objects.requireNonNull(filter);
+ boolean changed = false;
+ final Iterator<K> 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> 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<K> {
+ private final Iterator<Entry<K, V>> iterator;
+
+ private KeySetIterator(final Iterator<Entry<K, V>> 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<V> {
+
+ /** Generated serial version ID. */
+ private static final long serialVersionUID = 1L;
+
+ private ValuesCollection(final Collection<V> coll) {
+ super(coll);
+ }
+
+ @Override
+ public Iterator<V> 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<V> it = iterator();
+ while (it.hasNext()) {
+ if (Objects.equals(it.next(), value)) {
+ it.remove();
+ return true;
+ }
+ }
+ return false;
+ }
+
+ @Override
+ public boolean removeAll(final Collection<?> coll) {
+ Objects.requireNonNull(coll);
+ 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) {
+ Objects.requireNonNull(coll);
+ 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<? super V> filter) {
+ Objects.requireNonNull(filter);
+ boolean changed = false;
+ final Iterator<V> 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> 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<V> {
+ private final Iterator<Entry<K, V>> iterator;
+
+ private ValuesIterator(final Iterator<Entry<K, V>> 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 df889d2e9..436d6cfdc 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,10 @@ import static org.junit.jupiter.api.Assertions.assertNull;
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.concurrent.TimeUnit;
@@ -255,4 +258,185 @@ public class PassiveExpiringMapTest<K, V> extends
AbstractMapTest<PassiveExpirin
assertNull(map.get("a"));
}
+ @Test
+ void testCollectionsSynchronizedMapExpiration() throws
InterruptedException {
+ final Map<String, String> map = Collections.synchronizedMap(new
PassiveExpiringMap<>(50L));
+ map.put("a", "b");
+ map.put("c", "d");
+ assertEquals(2, map.size());
+ // Cache the views in SynchronizedMap before they expire
+ final Collection<Map.Entry<String, String>> entrySet = map.entrySet();
+ final Collection<String> keySet = map.keySet();
+ final Collection<String> values = map.values();
+ Thread.sleep(100L);
+ // entrySet view access triggers expiration
+ synchronized (map) {
+ assertTrue(entrySet.isEmpty());
+ assertTrue(keySet.isEmpty());
+ assertTrue(values.isEmpty());
+ }
+ map.put("a", "b");
+ map.put("c", "d");
+ assertEquals(2, map.size());
+ Thread.sleep(100L);
+ // keySet view access triggers expiration
+ synchronized (map) {
+ assertTrue(entrySet.isEmpty());
+ assertTrue(keySet.isEmpty());
+ assertTrue(values.isEmpty());
+ }
+ map.put("a", "b");
+ map.put("c", "d");
+ assertEquals(2, map.size());
+ Thread.sleep(100L);
+ // values view access triggers expiration
+ synchronized (map) {
+ assertTrue(entrySet.isEmpty());
+ assertTrue(keySet.isEmpty());
+ assertTrue(values.isEmpty());
+ }
+ }
+
+ @Test
+ void testCollectionViewRemoval() {
+ final PassiveExpiringMap<String, String> map = new
PassiveExpiringMap<>(10000L);
+ map.put("a", "b");
+ map.put("c", "d");
+ map.put("e", "f");
+ // Remove via entrySet iterator
+ final Iterator<Map.Entry<String, String>> entryIter =
map.entrySet().iterator();
+ assertTrue(entryIter.hasNext());
+ final Map.Entry<String, String> entry = entryIter.next();
+ final String removedKey = entry.getKey();
+ entryIter.remove();
+ assertFalse(map.containsKey(removedKey));
+ // Remove via keySet iterator
+ final Iterator<String> keyIter = map.keySet().iterator();
+ assertTrue(keyIter.hasNext());
+ final String key = keyIter.next();
+ keyIter.remove();
+ assertFalse(map.containsKey(key));
+ // Remove via values iterator
+ final Iterator<String> valIter = map.values().iterator();
+ assertTrue(valIter.hasNext());
+ final String val = valIter.next();
+ valIter.remove();
+ assertFalse(map.containsValue(val));
+ }
+
+ @Test
+ void testCollectionViewNullInputs() {
+ final PassiveExpiringMap<String, String> map = new
PassiveExpiringMap<>(10000L);
+ map.put("a", "b");
+ // entrySet
+ assertThrows(NullPointerException.class, () ->
map.entrySet().removeAll(null));
+ assertThrows(NullPointerException.class, () ->
map.entrySet().retainAll(null));
+ // keySet
+ assertThrows(NullPointerException.class, () ->
map.keySet().removeAll(null));
+ assertThrows(NullPointerException.class, () ->
map.keySet().retainAll(null));
+ // values
+ assertThrows(NullPointerException.class, () ->
map.values().removeAll(null));
+ assertThrows(NullPointerException.class, () ->
map.values().retainAll(null));
+ }
+
+ @SuppressWarnings("unchecked")
+ private Map<Object, Long> getExpirationMap(final PassiveExpiringMap<?, ?>
map) {
+ try {
+ final java.lang.reflect.Field field =
PassiveExpiringMap.class.getDeclaredField("expirationMap");
+ field.setAccessible(true);
+ return (Map<Object, Long>) field.get(map);
+ } catch (final Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Test
+ void testExpirationMapCleanup() {
+ final PassiveExpiringMap<String, String> map = new
PassiveExpiringMap<>(10000L);
+ final Map<Object, Long> expirationMap = getExpirationMap(map);
+
+ // Verify initial size
+ assertEquals(0, expirationMap.size());
+
+ // Verify cleanup on put and remove
+ map.put("a", "b");
+ map.put("c", "d");
+ assertEquals(2, expirationMap.size());
+ map.remove("a");
+ assertEquals(1, expirationMap.size());
+ assertFalse(expirationMap.containsKey("a"));
+
+ // Verify cleanup on clear
+ map.put("a", "b");
+ assertEquals(2, expirationMap.size());
+ map.clear();
+ assertEquals(0, expirationMap.size());
+
+ // Verify cleanup on entrySet remove
+ map.put("a", "b");
+ map.put("c", "d");
+ assertEquals(2, expirationMap.size());
+ map.entrySet().remove(map.entrySet().iterator().next());
+ assertEquals(1, expirationMap.size());
+
+ // Verify cleanup on keySet remove
+ map.put("e", "f");
+ assertEquals(2, expirationMap.size());
+ map.keySet().remove("e");
+ assertEquals(1, expirationMap.size());
+ assertFalse(expirationMap.containsKey("e"));
+
+ // Verify cleanup on values remove
+ map.put("g", "h");
+ assertEquals(2, expirationMap.size());
+ map.values().remove("h");
+ assertEquals(1, expirationMap.size());
+ assertFalse(expirationMap.containsKey("g"));
+
+ // Verify cleanup on removeAll
+ map.put("i", "j");
+ map.put("k", "l");
+ assertEquals(3, expirationMap.size());
+ map.keySet().removeAll(Collections.singleton("i"));
+ assertEquals(2, expirationMap.size());
+ assertFalse(expirationMap.containsKey("i"));
+
+ // Verify cleanup on retainAll
+ map.keySet().retainAll(Collections.singleton("k"));
+ assertEquals(1, expirationMap.size());
+ assertTrue(expirationMap.containsKey("k"));
+
+ // Verify cleanup on removeIf
+ map.keySet().removeIf(k -> k.equals("k"));
+ assertEquals(0, expirationMap.size());
+
+ // Verify cleanup on iterator remove
+ map.put("a", "b");
+ map.put("c", "d");
+ assertEquals(2, expirationMap.size());
+ final Iterator<String> iterator = map.keySet().iterator();
+ assertTrue(iterator.hasNext());
+ final String removedKey = iterator.next();
+ iterator.remove();
+ assertEquals(1, expirationMap.size());
+ assertFalse(expirationMap.containsKey(removedKey));
+ }
+
+ @Test
+ void testCollectionViewIteratorExpiration() throws InterruptedException {
+ final PassiveExpiringMap<String, String> map = new
PassiveExpiringMap<>(50L);
+ map.put("a", "b");
+
+ final Collection<Map.Entry<String, String>> entrySet = map.entrySet();
+ final Collection<String> keySet = map.keySet();
+ final Collection<String> 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());
+ }
+
}
