From b1273943835c8002c7aae24b880f2038fa71e73c Mon Sep 17 00:00:00 2001 From: Kristijan Mitrovic Date: Wed, 3 Jun 2020 02:04:16 +0000 Subject: [PATCH] Fix expiration in ExpirationMap (#2) Fix expiration time check in ExpirationMap Add helper methods for checking past and future timestamp Co-authored-by: Kristijan Mitrovic Reviewed-on: https://git.bubblev.org/bubblev/cobbzilla-utils/pulls/2 --- .../util/collection/ExpirationMap.java | 48 ++++++++++++++----- .../org/cobbzilla/util/time/TimeUtil.java | 2 + .../util/collection/ExpirationMapTest.java | 40 ++++++++++++++++ 3 files changed, 78 insertions(+), 12 deletions(-) create mode 100644 src/test/java/org/cobbzilla/util/collection/ExpirationMapTest.java diff --git a/src/main/java/org/cobbzilla/util/collection/ExpirationMap.java b/src/main/java/org/cobbzilla/util/collection/ExpirationMap.java index a182aad..b5cdd82 100644 --- a/src/main/java/org/cobbzilla/util/collection/ExpirationMap.java +++ b/src/main/java/org/cobbzilla/util/collection/ExpirationMap.java @@ -17,6 +17,8 @@ import java.util.stream.Collectors; import static org.cobbzilla.util.daemon.ZillaRuntime.notSupported; import static org.cobbzilla.util.daemon.ZillaRuntime.now; +import static org.cobbzilla.util.time.TimeUtil.isTimestampInFuture; +import static org.cobbzilla.util.time.TimeUtil.isTimestampInPast; @Accessors(chain=true) public class ExpirationMap implements Map { @@ -28,12 +30,19 @@ public class ExpirationMap implements Map { @Getter @Setter private long cleanInterval = TimeUnit.HOURS.toMillis(4); public ExpirationMap setExpirations(long val) { + final var isNewExpirationShorter = val < this.expiration; this.expiration = this.maxExpiration = this.cleanInterval = val; + if (isNewExpirationShorter) { + final var updatedNextCleaningTime = now() + this.expiration; + // the following calculation of nextCleaningTime is not really correct, but it doesn't really influence + // anything much: + if (this.nextCleaningTime > updatedNextCleaningTime) this.nextCleaningTime = updatedNextCleaningTime; + } return this; } @Getter @Setter private ExpirationEvictionPolicy evictionPolicy = ExpirationEvictionPolicy.ctime_or_atime; - private long lastCleaned = 0; + private long nextCleaningTime = now(); public ExpirationMap() { this.map = new ConcurrentHashMap<>(); } @@ -66,13 +75,23 @@ public class ExpirationMap implements Map { } } - @Override public int size() { return map.size(); } + @Override public int size() { + if (isTimestampInPast(nextCleaningTime)) cleanExpired(); + return map.size(); + } - @Override public boolean isEmpty() { return map.isEmpty(); } + @Override public boolean isEmpty() { + if (isTimestampInPast(nextCleaningTime)) cleanExpired(); + return map.isEmpty(); + } - @Override public boolean containsKey(Object key) { return map.containsKey(key); } + @Override public boolean containsKey(Object key) { + if (isTimestampInPast(nextCleaningTime)) cleanExpired(); + return map.containsKey(key); + } @Override public boolean containsValue(Object value) { + if (isTimestampInPast(nextCleaningTime)) cleanExpired(); for (ExpirationMapEntry val : map.values()) { if (val.value == value) return true; } @@ -80,13 +99,13 @@ public class ExpirationMap implements Map { } @Override public V get(Object key) { - if (lastCleaned+cleanInterval > now()) cleanExpired(); + if (isTimestampInPast(nextCleaningTime)) cleanExpired(); final ExpirationMapEntry value = map.get(key); return value == null || value.expired() ? null : value.touch().value; } @Override public V put(K key, V value) { - if (lastCleaned+cleanInterval > now()) cleanExpired(); + if (isTimestampInPast(nextCleaningTime)) cleanExpired(); final ExpirationMapEntry previous = map.put(key, new ExpirationMapEntry<>(value)); return previous == null ? null : previous.value; } @@ -104,25 +123,29 @@ public class ExpirationMap implements Map { @Override public void clear() { map.clear(); } - @Override public Set keySet() { return map.keySet(); } + @Override public Set keySet() { + if (isTimestampInPast(nextCleaningTime)) cleanExpired(); + return map.keySet(); + } @Override public Collection values() { + if (isTimestampInPast(nextCleaningTime)) cleanExpired(); return map.values().stream().map(v -> v.value).collect(Collectors.toList()); } @Override public V putIfAbsent(K key, V value) { - if (lastCleaned+cleanInterval > now()) cleanExpired(); + if (isTimestampInPast(nextCleaningTime)) cleanExpired(); final ExpirationMapEntry val = map.putIfAbsent(key, new ExpirationMapEntry<>(value)); return val == null ? null : val.value; } @Override public V computeIfAbsent(K key, Function mappingFunction) { - if (lastCleaned+cleanInterval > now()) cleanExpired(); + if (isTimestampInPast(nextCleaningTime)) cleanExpired(); return map.computeIfAbsent(key, k -> new ExpirationMapEntry<>(mappingFunction.apply(k))).value; } @Override public V computeIfPresent(K key, BiFunction remappingFunction) { - if (lastCleaned+cleanInterval > now()) cleanExpired(); + if (isTimestampInPast(nextCleaningTime)) cleanExpired(); final ExpirationMapEntry found = map.computeIfPresent(key, (k, vExpirationMapEntry) -> new ExpirationMapEntry<>(remappingFunction.apply(k, vExpirationMapEntry.value))); return found == null ? null : found.value; } @@ -135,12 +158,13 @@ public class ExpirationMap implements Map { } @Override public Set> entrySet() { + if (isTimestampInPast(nextCleaningTime)) cleanExpired(); return map.entrySet().stream().map(e -> new EMEntry<>(e.getKey(), e.getValue().value)).collect(Collectors.toSet()); } private synchronized void cleanExpired () { - if (lastCleaned+cleanInterval < now()) return; - lastCleaned = now(); + if (isTimestampInFuture(nextCleaningTime)) return; + nextCleaningTime = now() + cleanInterval; final Set toRemove = new HashSet<>(); for (Map.Entry> entry : map.entrySet()) { if (entry.getValue().expired()) toRemove.add(entry.getKey()); diff --git a/src/main/java/org/cobbzilla/util/time/TimeUtil.java b/src/main/java/org/cobbzilla/util/time/TimeUtil.java index 9b830aa..6adebe4 100644 --- a/src/main/java/org/cobbzilla/util/time/TimeUtil.java +++ b/src/main/java/org/cobbzilla/util/time/TimeUtil.java @@ -223,4 +223,6 @@ public class TimeUtil { return new DateTime(zone).withTimeAtStartOfDay().withFieldAdded(DurationFieldType.years(), -1).withDayOfYear(1); } + public static boolean isTimestampInFuture(long t) { return t > now(); } + public static boolean isTimestampInPast(long t) { return t < now(); } } diff --git a/src/test/java/org/cobbzilla/util/collection/ExpirationMapTest.java b/src/test/java/org/cobbzilla/util/collection/ExpirationMapTest.java new file mode 100644 index 0000000..e09f06f --- /dev/null +++ b/src/test/java/org/cobbzilla/util/collection/ExpirationMapTest.java @@ -0,0 +1,40 @@ +package org.cobbzilla.util.collection; + +import junit.framework.TestCase; + +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.cobbzilla.util.daemon.ZillaRuntime.now; +import static org.cobbzilla.util.system.Sleep.sleep; + +public class ExpirationMapTest extends TestCase { + + public void testExpiration() { + final var halfExpiration = SECONDS.toMillis(1); + final var map = new ExpirationMap(halfExpiration * 2); + + map.put("t1", now()); + assertEquals(1, map.size()); + assertNotNull(map.get("t1")); + assertNotNull(map.get("t1")); // reading cached again + + sleep(halfExpiration); + + map.put("t2", now()); + assertEquals(2, map.size()); + assertNotNull(map.get("t1")); + assertNotNull(map.get("t1")); // reading cached again + assertNotNull(map.get("t2")); + assertNotNull(map.get("t2")); // reading cached again + + sleep(halfExpiration + 1); // add 1ms just in case ... + + assertEquals(1, map.size()); + assertNotNull(map.get("t2")); + assertNotNull(map.get("t2")); // reading cached again + + // note that previous cleaning has been just done, and so waiting for another full interval here + // (+1ms just in case) + sleep(halfExpiration * 2 + 1); + assertEquals(0, map.size()); + } +} \ No newline at end of file