Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Commit

Permalink
Fix LruCache corruption bug with a size_callback that can return 0 (
Browse files Browse the repository at this point in the history
#11454)

When all entries in an `LruCache` have a size of 0 according to the
provided `size_callback`, and `drop_from_cache` is called on a cache
node, the node would be unlinked from the LRU linked list but remain in
the cache dictionary. An assertion would be later be tripped due to the
inconsistency.

Avoid unintentionally calling `__len__` and use a strict `is None`
check instead when unwrapping the weak reference.
  • Loading branch information
squahtx authored Nov 30, 2021
1 parent 5a0b652 commit 7ff22d6
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 1 deletion.
1 change: 1 addition & 0 deletions changelog.d/11454.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix an `LruCache` corruption bug, introduced in 1.38.0, that would cause certain requests to fail until the next Synapse restart.
5 changes: 4 additions & 1 deletion synapse/util/caches/lrucache.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,10 @@ def drop_from_cache(self) -> None:
removed from all lists.
"""
cache = self._cache()
if not cache or not cache.pop(self.key, None):
if (
cache is None
or cache.pop(self.key, _Sentinel.sentinel) is _Sentinel.sentinel
):
# `cache.pop` should call `drop_from_lists()`, unless this Node had
# already been removed from the cache.
self.drop_from_lists()
Expand Down
12 changes: 12 additions & 0 deletions tests/util/test_lrucache.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.


from typing import List
from unittest.mock import Mock

from synapse.util.caches.lrucache import LruCache, setup_expire_lru_cache_entries
Expand Down Expand Up @@ -261,6 +262,17 @@ def test_evict(self):
self.assertEquals(cache["key4"], [4])
self.assertEquals(cache["key5"], [5, 6])

def test_zero_size_drop_from_cache(self) -> None:
"""Test that `drop_from_cache` works correctly with 0-sized entries."""
cache: LruCache[str, List[int]] = LruCache(5, size_callback=lambda x: 0)
cache["key1"] = []

self.assertEqual(len(cache), 0)
cache.cache["key1"].drop_from_cache()
self.assertIsNone(
cache.pop("key1"), "Cache entry should have been evicted but wasn't"
)


class TimeEvictionTestCase(unittest.HomeserverTestCase):
"""Test that time based eviction works correctly."""
Expand Down

0 comments on commit 7ff22d6

Please sign in to comment.