From cfae8c9df28592baab1f5b1f7ac58fd72084dc13 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Mon, 29 Nov 2021 21:36:47 +0000 Subject: [PATCH 1/7] Fix `LruCache` corruption bug with a `size_callback` that can return 0 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. Use a strict `is None` check instead when unwrapping the weak reference. --- synapse/util/caches/lrucache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/util/caches/lrucache.py b/synapse/util/caches/lrucache.py index a0a7a9de3299..d5670263f329 100644 --- a/synapse/util/caches/lrucache.py +++ b/synapse/util/caches/lrucache.py @@ -270,7 +270,7 @@ 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 not cache.pop(self.key, None): # `cache.pop` should call `drop_from_lists()`, unless this Node had # already been removed from the cache. self.drop_from_lists() From 3c86fae7b30ac97127c758dbb038d282aa4601c6 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Mon, 29 Nov 2021 21:49:59 +0000 Subject: [PATCH 2/7] Add questionable unit test --- tests/util/test_lrucache.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/util/test_lrucache.py b/tests/util/test_lrucache.py index 6578f3411e27..329616aac916 100644 --- a/tests/util/test_lrucache.py +++ b/tests/util/test_lrucache.py @@ -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 @@ -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.""" From a136975a19426acf0d144cfe297ee179b1eafaa1 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Mon, 29 Nov 2021 21:51:07 +0000 Subject: [PATCH 3/7] Add newsfile --- changelog.d/11454.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11454.bugfix diff --git a/changelog.d/11454.bugfix b/changelog.d/11454.bugfix new file mode 100644 index 000000000000..c9a28fffa427 --- /dev/null +++ b/changelog.d/11454.bugfix @@ -0,0 +1 @@ +Fix a long-standing `LruCache` corruption bug that would cause requests to fail. From 14e816ec01d84d09375222bf6446cb53d52b43a7 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Mon, 29 Nov 2021 21:53:03 +0000 Subject: [PATCH 4/7] Tweak docstring --- tests/util/test_lrucache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/util/test_lrucache.py b/tests/util/test_lrucache.py index 329616aac916..291644eb7dd0 100644 --- a/tests/util/test_lrucache.py +++ b/tests/util/test_lrucache.py @@ -263,7 +263,7 @@ def test_evict(self): 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""" + """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"] = [] From b1a1c9bfc1c6596fe33a09b076de21163c9a25a1 Mon Sep 17 00:00:00 2001 From: Sean Quah <8349537+squahtx@users.noreply.github.com> Date: Tue, 30 Nov 2021 12:15:47 +0000 Subject: [PATCH 5/7] Update synapse/util/caches/lrucache.py Co-authored-by: reivilibre --- synapse/util/caches/lrucache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/util/caches/lrucache.py b/synapse/util/caches/lrucache.py index d5670263f329..eb9b870acc2a 100644 --- a/synapse/util/caches/lrucache.py +++ b/synapse/util/caches/lrucache.py @@ -270,7 +270,7 @@ def drop_from_cache(self) -> None: removed from all lists. """ cache = self._cache() - if cache is None or not cache.pop(self.key, None): + if cache is None or cache.pop(self.key, None) is None: # `cache.pop` should call `drop_from_lists()`, unless this Node had # already been removed from the cache. self.drop_from_lists() From 1971a1328bce1e9b5cb57f31dee4444538cfe31e Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Tue, 30 Nov 2021 15:41:49 +0000 Subject: [PATCH 6/7] Use sentinel value in `drop_from_cache` check --- synapse/util/caches/lrucache.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/synapse/util/caches/lrucache.py b/synapse/util/caches/lrucache.py index 7fbde6f721fe..eb96f7e665e6 100644 --- a/synapse/util/caches/lrucache.py +++ b/synapse/util/caches/lrucache.py @@ -271,7 +271,10 @@ def drop_from_cache(self) -> None: removed from all lists. """ cache = self._cache() - if cache is None or cache.pop(self.key, None) is 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() From 6a9dc2dde3a8a1528b95ce04edb17b6953571379 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Tue, 30 Nov 2021 15:57:56 +0000 Subject: [PATCH 7/7] Update newsfile --- changelog.d/11454.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/11454.bugfix b/changelog.d/11454.bugfix index c9a28fffa427..096265cbc933 100644 --- a/changelog.d/11454.bugfix +++ b/changelog.d/11454.bugfix @@ -1 +1 @@ -Fix a long-standing `LruCache` corruption bug that would cause requests to fail. +Fix an `LruCache` corruption bug, introduced in 1.38.0, that would cause certain requests to fail until the next Synapse restart.