From 7729b357298bd822b163f724d82535b9cf24ddf5 Mon Sep 17 00:00:00 2001 From: John DeRegnaucourt Date: Sat, 22 Jun 2024 10:21:42 -0400 Subject: [PATCH] - LRUCache completely re-written to remove potential memory leak. --- README.md | 4 +- changelog.md | 4 + .../java/com/cedarsoftware/util/LRUCache.java | 347 ++++++++++-------- .../com/cedarsoftware/util/LRUCacheTest.java | 186 +++++++++- 4 files changed, 391 insertions(+), 150 deletions(-) diff --git a/README.md b/README.md index b9037442..fe2d25a3 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ Both of these features ensure that our library can be seamlessly integrated into To include in your project: ##### Gradle ```groovy -implementation 'com.cedarsoftware:java-util:2.9.0' +implementation 'com.cedarsoftware:java-util:2.10.0' ``` ##### Maven @@ -33,7 +33,7 @@ implementation 'com.cedarsoftware:java-util:2.9.0' com.cedarsoftware java-util - 2.9.0 + 2.10.0 ``` --- diff --git a/changelog.md b/changelog.md index 69de2261..0e96e333 100644 --- a/changelog.md +++ b/changelog.md @@ -1,4 +1,8 @@ ### Revision History +* 2.10.0 + * Fixed potential memory leak in `LRUCache.` + * Added `nextPermutation` to `MathUtilities.` + * Added `size(),`, `isEmpty(),` and `hasContent` to `CollectionUtilities.` * 2.9.0 * Added `SealableList` which provides a `List` (or `List` wrapper) that will make it read-only (sealed) or read-write (unsealed), controllable via a `Supplier.` This moves the immutability control outside the list and ensures that all views on the `List` respect the sealed-ness. One master supplier can control the immutability of many collections. * Added `SealableSet` similar to SealableList but with `Set` nature. diff --git a/src/main/java/com/cedarsoftware/util/LRUCache.java b/src/main/java/com/cedarsoftware/util/LRUCache.java index d1ae6248..8ea9de82 100644 --- a/src/main/java/com/cedarsoftware/util/LRUCache.java +++ b/src/main/java/com/cedarsoftware/util/LRUCache.java @@ -1,13 +1,15 @@ package com.cedarsoftware.util; +import java.util.AbstractMap; +import java.util.ArrayList; import java.util.Collection; -import java.util.Iterator; +import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Objects; import java.util.Set; -import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.locks.ReentrantReadWriteLock; -import java.util.function.Supplier; /** * This class provides a thread-safe Least Recently Used (LRU) cache API that will evict the least recently used items, @@ -31,169 +33,220 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -public class LRUCache implements Map { - private final Map cache; - private final transient ReadWriteLock lock = new ReentrantReadWriteLock(); - private final static Object NO_ENTRY = new Object(); +public class LRUCache extends AbstractMap implements Map { + private final Map map; + private final Node head; + private final Node tail; + private final int capacity; + private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); + + private class Node { + K key; + V value; + Node prev; + Node next; + + Node(K key, V value) { + this.key = key; + this.value = value; + } + + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Node node = (Node) o; + return Objects.equals(key, node.key) && Objects.equals(value, node.value); + } + + public int hashCode() { + return Objects.hash(key, value); + } + + public String toString() { + return "Node{" + + "key=" + key + + ", value=" + value + + '}'; + } + } public LRUCache(int capacity) { - cache = new LinkedHashMap(capacity, 0.75f, true) { - protected boolean removeEldestEntry(Map.Entry eldest) { - return size() > capacity; - } - }; - } - - // Immutable APIs - public boolean equals(Object obj) { return readOperation(() -> cache.equals(obj)); } - public int hashCode() { return readOperation(cache::hashCode); } - public String toString() { return readOperation(cache::toString); } - public int size() { return readOperation(cache::size); } - public boolean isEmpty() { return readOperation(cache::isEmpty); } - public boolean containsKey(Object key) { return readOperation(() -> cache.containsKey(key)); } - public boolean containsValue(Object value) { return readOperation(() -> cache.containsValue(value)); } - public V get(Object key) { return readOperation(() -> cache.get(key)); } - - // Mutable APIs - public V put(K key, V value) { return writeOperation(() -> cache.put(key, value)); } - public void putAll(Map m) { writeOperation(() -> { cache.putAll(m); return null; }); } - public V putIfAbsent(K key, V value) { return writeOperation(() -> cache.putIfAbsent(key, value)); } - public V remove(Object key) { return writeOperation(() -> cache.remove(key)); } - public void clear() { writeOperation(() -> { cache.clear(); return null; }); } + this.capacity = capacity; + this.map = new ConcurrentHashMap<>(capacity); + this.head = new Node(null, null); + this.tail = new Node(null, null); + head.next = tail; + tail.prev = head; + } - public Set keySet() { - return readOperation(() -> new Set() { - // Immutable APIs - public int size() { return readOperation(cache::size); } - public boolean isEmpty() { return readOperation(cache::isEmpty); } - public boolean contains(Object o) { return readOperation(() -> cache.containsKey(o)); } - public boolean containsAll(Collection c) { return readOperation(() -> cache.keySet().containsAll(c)); } - public Object[] toArray() { return readOperation(() -> cache.keySet().toArray()); } - public T[] toArray(T[] a) { return readOperation(() -> cache.keySet().toArray(a)); } - - // Mutable APIs - public Iterator iterator() { - return new Iterator() { - private final Iterator it = cache.keySet().iterator(); - private K current = (K)NO_ENTRY; - - public boolean hasNext() { return readOperation(it::hasNext); } - public K next() { return readOperation(() -> { current = it.next(); return current; }); } - public void remove() { - writeOperation(() -> { - if (current == NO_ENTRY) { - throw new IllegalStateException("Next not called or key already removed"); - } - it.remove(); // Remove from the underlying map - current = (K)NO_ENTRY; - return null; - }); - } - }; + public V get(Object key) { + lock.readLock().lock(); + try { + Node node = map.get(key); + if (node == null) { + return null; } - public boolean add(K k) { throw new UnsupportedOperationException("add() not supported on .keySet() of a Map"); } - public boolean remove(Object o) { return writeOperation(() -> cache.remove(o) != null); } - public boolean addAll(Collection c) { throw new UnsupportedOperationException("addAll() not supported on .keySet() of a Map"); } - public boolean retainAll(Collection c) { return writeOperation(() -> cache.keySet().retainAll(c)); } - public boolean removeAll(Collection c) { return writeOperation(() -> cache.keySet().removeAll(c)); } - public void clear() { writeOperation(() -> { cache.clear(); return null; }); } - }); + moveToHead(node); + return node.value; + } finally { + lock.readLock().unlock(); + } } - public Collection values() { - return readOperation(() -> new Collection() { - // Immutable APIs - public int size() { return readOperation(cache::size); } - public boolean isEmpty() { return readOperation(cache::isEmpty); } - public boolean contains(Object o) { return readOperation(() -> cache.containsValue(o)); } - public boolean containsAll(Collection c) { return readOperation(() -> cache.values().containsAll(c)); } - public Object[] toArray() { return readOperation(() -> cache.values().toArray()); } - public T[] toArray(T[] a) { return readOperation(() -> cache.values().toArray(a)); } - - // Mutable APIs - public Iterator iterator() { - return new Iterator() { - private final Iterator it = cache.values().iterator(); - private V current = (V)NO_ENTRY; - - public boolean hasNext() { return readOperation(it::hasNext); } - public V next() { return readOperation(() -> { current = it.next(); return current; }); } - public void remove() { - writeOperation(() -> { - if (current == NO_ENTRY) { - throw new IllegalStateException("Next not called or entry already removed"); - } - it.remove(); // Remove from the underlying map - current = (V)NO_ENTRY; - return null; - }); - } - }; + public V put(K key, V value) { + lock.writeLock().lock(); + try { + Node newNode = new Node(key, value); + Node oldNode = map.put(key, newNode); + + if (oldNode != null) { + removeNode(oldNode); } - public boolean add(V value) { throw new UnsupportedOperationException("add() not supported on values() of a Map"); } - public boolean remove(Object o) { return writeOperation(() -> cache.values().remove(o)); } - public boolean addAll(Collection c) { throw new UnsupportedOperationException("addAll() not supported on values() of a Map"); } - public boolean removeAll(Collection c) { return writeOperation(() -> cache.values().removeAll(c)); } - public boolean retainAll(Collection c) { return writeOperation(() -> cache.values().retainAll(c)); } - public void clear() { writeOperation(() -> { cache.clear(); return null; }); } - }); - } - public Set> entrySet() { - return readOperation(() -> new Set>() { - // Immutable APIs - public int size() { return readOperation(cache::size); } - public boolean isEmpty() { return readOperation(cache::isEmpty); } - public boolean contains(Object o) { return readOperation(() -> cache.entrySet().contains(o)); } - public boolean containsAll(Collection c) { return readOperation(() -> cache.entrySet().containsAll(c)); } - public Object[] toArray() { return readOperation(() -> cache.entrySet().toArray()); } - public T[] toArray(T[] a) { return readOperation(() -> cache.entrySet().toArray(a)); } - - // Mutable APIs - public Iterator> iterator() { - return new Iterator>() { - private final Iterator> it = cache.entrySet().iterator(); - private Entry current = (Entry) NO_ENTRY; - - public boolean hasNext() { return readOperation(it::hasNext); } - public Entry next() { return readOperation(() -> { current = it.next(); return current; }); } - public void remove() { - writeOperation(() -> { - if (current == NO_ENTRY) { - throw new IllegalStateException("Next not called or entry already removed"); - } - it.remove(); - current = (Entry) NO_ENTRY; - return null; - }); - } - }; + addToHead(newNode); + + if (map.size() > capacity) { + Node oldestNode = removeTailNode(); + if (oldestNode != null) { + map.remove(oldestNode.key); + } } - public boolean add(Entry kvEntry) { throw new UnsupportedOperationException("add() not supported on entrySet() of a Map"); } - public boolean remove(Object o) { return writeOperation(() -> cache.entrySet().remove(o)); } - public boolean addAll(Collection> c) { throw new UnsupportedOperationException("addAll() not supported on entrySet() of a Map"); } - public boolean retainAll(Collection c) { return writeOperation(() -> cache.entrySet().retainAll(c)); } - public boolean removeAll(Collection c) { return writeOperation(() -> cache.entrySet().removeAll(c)); } - public void clear() { writeOperation(() -> { cache.clear(); return null; }); } - }); + return oldNode != null ? oldNode.value : null; + } finally { + lock.writeLock().unlock(); + } } - - private T readOperation(Supplier operation) { - lock.readLock().lock(); + + public V remove(Object key) { + lock.writeLock().lock(); try { - return operation.get(); + Node node = map.remove(key); + if (node != null) { + removeNode(node); + return node.value; + } + return null; } finally { - lock.readLock().unlock(); + lock.writeLock().unlock(); } } - private T writeOperation(Supplier operation) { + public void clear() { lock.writeLock().lock(); try { - return operation.get(); + map.clear(); + head.next = tail; + tail.prev = head; } finally { lock.writeLock().unlock(); } } -} + + public int size() { + return map.size(); + } + + public boolean containsKey(Object key) { + return map.containsKey(key); + } + + public boolean containsValue(Object value) { + for (Node node : map.values()) { + if (Objects.equals(node.value, value)) { + return true; + } + } + return false; + } + + public Set> entrySet() { + Map result = new LinkedHashMap<>(); + for (Node node : map.values()) { + result.put(node.key, node.value); + } + return Collections.unmodifiableSet(result.entrySet()); + } + + public Set keySet() { + return Collections.unmodifiableSet(map.keySet()); + } + + public Collection values() { + Collection values = new ArrayList<>(); + for (Node node : map.values()) { + values.add(node.value); + } + return Collections.unmodifiableCollection(values); + } + + public boolean equals(Object o) { + if (o == this) { + return true; + } + if (o instanceof Map) { + Map other = (Map) o; + if (other.size() != this.size()) { + return false; + } + for (Map.Entry entry : other.entrySet()) { + V value = this.get(entry.getKey()); + if (!Objects.equals(value, entry.getValue())) { + return false; + } + } + return true; + } + return false; + } + + public int hashCode() { + int hashCode = 1; + for (Map.Entry entry : map.entrySet()) { + hashCode = 31 * hashCode + (entry.getKey() == null ? 0 : entry.getKey().hashCode()); + hashCode = 31 * hashCode + (entry.getValue().value == null ? 0 : entry.getValue().value.hashCode()); + } + return hashCode; + } + + public String toString() { + StringBuilder sb = new StringBuilder("{"); + for (Map.Entry entry : map.entrySet()) { + sb.append(entry.getKey()).append("=").append(entry.getValue().value).append(", "); + } + if (sb.length() > 1) { + sb.setLength(sb.length() - 2); + } + sb.append("}"); + return sb.toString(); + } + + private void addToHead(Node node) { + Node nextNode = head.next; + node.next = nextNode; + node.prev = head; + head.next = node; + nextNode.prev = node; + } + + private void removeNode(Node node) { + Node prevNode = node.prev; + Node nextNode = node.next; + prevNode.next = nextNode; + nextNode.prev = prevNode; + } + + private void moveToHead(Node node) { + removeNode(node); + addToHead(node); + } + + private Node removeTailNode() { + Node oldestNode = tail.prev; + if (oldestNode == head) { + return null; + } + removeNode(oldestNode); + return oldestNode; + } +} \ No newline at end of file diff --git a/src/test/java/com/cedarsoftware/util/LRUCacheTest.java b/src/test/java/com/cedarsoftware/util/LRUCacheTest.java index 2b0cd714..26112a49 100644 --- a/src/test/java/com/cedarsoftware/util/LRUCacheTest.java +++ b/src/test/java/com/cedarsoftware/util/LRUCacheTest.java @@ -14,9 +14,27 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +/** + * @author John DeRegnaucourt (jdereg@gmail.com) + *
+ * Copyright (c) Cedar Software LLC + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * License + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ public class LRUCacheTest { private LRUCache lruCache; @@ -143,11 +161,32 @@ void testPutIfAbsent() { assertEquals("A", lruCache.get(1)); } + + @Test + void testSmallSizes() + { + // Testing with different sizes + for (int capacity : new int[]{1, 3, 5, 10}) { + LRUCache cache = new LRUCache<>(capacity); + for (int i = 0; i < capacity; i++) { + cache.put(i, "Value" + i); + } + for (int i = 0; i < capacity; i++) { + cache.get(i); + } + for (int i = 0; i < capacity; i++) { + cache.remove(i); + } + + assert cache.isEmpty(); + cache.clear(); + } + } @Test void testConcurrency() throws InterruptedException { ExecutorService service = Executors.newFixedThreadPool(3); - lruCache = new LRUCache<>(100000); + lruCache = new LRUCache<>(10000); // Perform a mix of put and get operations from multiple threads int max = 10000; @@ -188,4 +227,149 @@ void testConcurrency() throws InterruptedException { // System.out.println("lruCache = " + lruCache.size()); // System.out.println("attempts =" + attempts); } + + @Test + public void testConcurrency2() throws InterruptedException { + int initialEntries = 100; + lruCache = new LRUCache<>(initialEntries); + ExecutorService executor = Executors.newFixedThreadPool(10); + + // Add initial entries + for (int i = 0; i < initialEntries; i++) { + lruCache.put(i, "true"); + } + + SecureRandom random = new SecureRandom(); + // Perform concurrent operations + for (int i = 0; i < 100000; i++) { + final int key = random.nextInt(100); + executor.submit(() -> { + lruCache.put(key, "true"); // Add + lruCache.remove(key); // Remove + lruCache.put(key, "false"); // Update + }); + } + + executor.shutdown(); + assertTrue(executor.awaitTermination(1, TimeUnit.MINUTES)); + + // Check some values to ensure correctness + for (int i = 0; i < initialEntries; i++) { + final int key = i; + assertTrue(lruCache.containsKey(key)); + } + + assert lruCache.size() == 100; + assertEquals(initialEntries, lruCache.size()); + } + + @Test + void testEquals() { + LRUCache cache1 = new LRUCache<>(3); + LRUCache cache2 = new LRUCache<>(3); + + cache1.put(1, "A"); + cache1.put(2, "B"); + cache1.put(3, "C"); + + cache2.put(1, "A"); + cache2.put(2, "B"); + cache2.put(3, "C"); + + assertTrue(cache1.equals(cache2)); + assertTrue(cache2.equals(cache1)); + + cache2.put(4, "D"); + assertFalse(cache1.equals(cache2)); + assertFalse(cache2.equals(cache1)); + } + + @Test + void testHashCode() { + LRUCache cache1 = new LRUCache<>(3); + LRUCache cache2 = new LRUCache<>(3); + + cache1.put(1, "A"); + cache1.put(2, "B"); + cache1.put(3, "C"); + + cache2.put(1, "A"); + cache2.put(2, "B"); + cache2.put(3, "C"); + + assertEquals(cache1.hashCode(), cache2.hashCode()); + + cache2.put(4, "D"); + assertNotEquals(cache1.hashCode(), cache2.hashCode()); + } + + @Test + void testToString() { + lruCache.put(1, "A"); + lruCache.put(2, "B"); + lruCache.put(3, "C"); + + String expected = "{1=A, 2=B, 3=C}"; + assertEquals(expected, lruCache.toString()); + } + + @Test + void testFullCycle() { + lruCache.put(1, "A"); + lruCache.put(2, "B"); + lruCache.put(3, "C"); + lruCache.put(4, "D"); + lruCache.put(5, "E"); + lruCache.put(6, "F"); + + String expected = "{4=D, 5=E, 6=F}"; + assertEquals(expected, lruCache.toString()); + + lruCache.remove(6); + lruCache.remove(5); + lruCache.remove(4); + assert lruCache.size() == 0; + } + + @Test + void testCacheWhenEmpty() { + // The cache is initially empty, so any get operation should return null + assertNull(lruCache.get(1)); + } + + @Test + void testCacheEvictionWhenCapacityExceeded() { + // Add elements to the cache until it reaches its capacity + lruCache.put(1, "A"); + lruCache.put(2, "B"); + lruCache.put(3, "C"); + + // Access an element to change the LRU order + lruCache.get(1); + + // Add another element to trigger eviction + lruCache.put(4, "D"); + + // Check that the least recently used element (2) was evicted + assertNull(lruCache.get(2)); + + // Check that the other elements are still in the cache + assertEquals("A", lruCache.get(1)); + assertEquals("C", lruCache.get(3)); + assertEquals("D", lruCache.get(4)); + } + + @Test + void testCacheClear() { + // Add elements to the cache + lruCache.put(1, "A"); + lruCache.put(2, "B"); + + // Clear the cache + lruCache.clear(); + + // The cache should be empty, so any get operation should return null + assertNull(lruCache.get(1)); + assertNull(lruCache.get(2)); + } }