summary refs log tree commit diff
diff options
context:
space:
mode:
authorSean Quah <8349537+squahtx@users.noreply.github.com>2021-11-30 16:28:02 +0000
committerGitHub <noreply@github.com>2021-11-30 16:28:02 +0000
commit7ff22d6da41cd5ca80db95c18b409aea38e49fcd (patch)
tree99dbc2a8eb19b7c1ecf23665970d706a995e7b91
parentEliminate a few `Any`s in `LruCache` type hints (#11453) (diff)
downloadsynapse-7ff22d6da41cd5ca80db95c18b409aea38e49fcd.tar.xz
Fix `LruCache` corruption bug with a `size_callback` that can return 0 (#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.
Diffstat (limited to '')
-rw-r--r--changelog.d/11454.bugfix1
-rw-r--r--synapse/util/caches/lrucache.py5
-rw-r--r--tests/util/test_lrucache.py12
3 files changed, 17 insertions, 1 deletions
diff --git a/changelog.d/11454.bugfix b/changelog.d/11454.bugfix
new file mode 100644
index 0000000000..096265cbc9
--- /dev/null
+++ b/changelog.d/11454.bugfix
@@ -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.
diff --git a/synapse/util/caches/lrucache.py b/synapse/util/caches/lrucache.py
index 05c4dcb062..eb96f7e665 100644
--- a/synapse/util/caches/lrucache.py
+++ b/synapse/util/caches/lrucache.py
@@ -271,7 +271,10 @@ class _Node(Generic[KT, VT]):
         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()
diff --git a/tests/util/test_lrucache.py b/tests/util/test_lrucache.py
index 6578f3411e..291644eb7d 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 @@ class LruCacheSizedTestCase(unittest.HomeserverTestCase):
         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."""