Empty iterables should count towards cache usage. (#9028)

This commit is contained in:
Patrick Cloke 2021-01-06 12:33:20 -05:00 committed by GitHub
parent 0312266ee3
commit 1b4d5d6acf
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 53 additions and 25 deletions

1
changelog.d/9028.bugfix Normal file
View file

@ -0,0 +1 @@
Fix a long-standing bug where some caches could grow larger than configured.

View file

@ -105,7 +105,7 @@ class DeferredCache(Generic[KT, VT]):
keylen=keylen, keylen=keylen,
cache_name=name, cache_name=name,
cache_type=cache_type, cache_type=cache_type,
size_callback=(lambda d: len(d)) if iterable else None, size_callback=(lambda d: len(d) or 1) if iterable else None,
metrics_collection_callback=metrics_cb, metrics_collection_callback=metrics_cb,
apply_cache_factor_from_config=apply_cache_factor_from_config, apply_cache_factor_from_config=apply_cache_factor_from_config,
) # type: LruCache[KT, VT] ) # type: LruCache[KT, VT]

View file

@ -25,13 +25,8 @@ from tests.unittest import TestCase
class DeferredCacheTestCase(TestCase): class DeferredCacheTestCase(TestCase):
def test_empty(self): def test_empty(self):
cache = DeferredCache("test") cache = DeferredCache("test")
failed = False with self.assertRaises(KeyError):
try:
cache.get("foo") cache.get("foo")
except KeyError:
failed = True
self.assertTrue(failed)
def test_hit(self): def test_hit(self):
cache = DeferredCache("test") cache = DeferredCache("test")
@ -155,13 +150,8 @@ class DeferredCacheTestCase(TestCase):
cache.prefill(("foo",), 123) cache.prefill(("foo",), 123)
cache.invalidate(("foo",)) cache.invalidate(("foo",))
failed = False with self.assertRaises(KeyError):
try:
cache.get(("foo",)) cache.get(("foo",))
except KeyError:
failed = True
self.assertTrue(failed)
def test_invalidate_all(self): def test_invalidate_all(self):
cache = DeferredCache("testcache") cache = DeferredCache("testcache")
@ -215,13 +205,8 @@ class DeferredCacheTestCase(TestCase):
cache.prefill(2, "two") cache.prefill(2, "two")
cache.prefill(3, "three") # 1 will be evicted cache.prefill(3, "three") # 1 will be evicted
failed = False with self.assertRaises(KeyError):
try:
cache.get(1) cache.get(1)
except KeyError:
failed = True
self.assertTrue(failed)
cache.get(2) cache.get(2)
cache.get(3) cache.get(3)
@ -239,13 +224,55 @@ class DeferredCacheTestCase(TestCase):
cache.prefill(3, "three") cache.prefill(3, "three")
failed = False with self.assertRaises(KeyError):
try:
cache.get(2) cache.get(2)
except KeyError:
failed = True
self.assertTrue(failed)
cache.get(1) cache.get(1)
cache.get(3) cache.get(3)
def test_eviction_iterable(self):
cache = DeferredCache(
"test", max_entries=3, apply_cache_factor_from_config=False, iterable=True,
)
cache.prefill(1, ["one", "two"])
cache.prefill(2, ["three"])
# Now access 1 again, thus causing 2 to be least-recently used
cache.get(1)
# Now add an item to the cache, which evicts 2.
cache.prefill(3, ["four"])
with self.assertRaises(KeyError):
cache.get(2)
# Ensure 1 & 3 are in the cache.
cache.get(1)
cache.get(3)
# Now access 1 again, thus causing 3 to be least-recently used
cache.get(1)
# Now add an item with multiple elements to the cache
cache.prefill(4, ["five", "six"])
# Both 1 and 3 are evicted since there's too many elements.
with self.assertRaises(KeyError):
cache.get(1)
with self.assertRaises(KeyError):
cache.get(3)
# Now add another item to fill the cache again.
cache.prefill(5, ["seven"])
# Now access 4, thus causing 5 to be least-recently used
cache.get(4)
# Add an empty item.
cache.prefill(6, [])
# 5 gets evicted and replaced since an empty element counts as an item.
with self.assertRaises(KeyError):
cache.get(5)
cache.get(4)
cache.get(6)