diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 75dcfde79e..9b8ff3f84d 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -24,6 +24,7 @@ + Wrapping PassiveExpiringMap in a SynchronizedMap breaks expiration. [javadoc] Document that BloomFilter merge operations are non-atomic and a filter that throws during a merge should be considered invalid. 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. 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..87a33c128c 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.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,102 @@ 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()); + // 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) { + 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 iterator 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 iterator triggers expiration + synchronized (map) { + assertTrue(entrySet.isEmpty()); + assertTrue(keySet.isEmpty()); + assertTrue(values.isEmpty()); + } + } + + @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)); + } + + @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)); + } + + @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()); + } + }