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

Fix LruCache corruption bug with a size_callback that can return 0 #11454

Merged
merged 8 commits into from
Nov 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that we can store None as a value in the cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the thinking, yes.

Although drop_from_lists is idempotent so it's functionally the same

):
# `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