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

Commit 21fea6b

Browse files
authored
Prefill events after invalidate not before when persisting events (#15758)
Fixes #15757
1 parent 8ddb2de commit 21fea6b

File tree

5 files changed

+70
-5
lines changed

5 files changed

+70
-5
lines changed

changelog.d/15758.bugfix

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Avoid invalidating a cache that was just prefilled.

synapse/storage/databases/main/events.py

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1729,13 +1729,22 @@ def _add_to_cache(
17291729
if not row["rejects"] and not row["redacts"]:
17301730
to_prefill.append(EventCacheEntry(event=event, redacted_event=None))
17311731

1732-
async def prefill() -> None:
1732+
async def external_prefill() -> None:
17331733
for cache_entry in to_prefill:
1734-
await self.store._get_event_cache.set(
1734+
await self.store._get_event_cache.set_external(
17351735
(cache_entry.event.event_id,), cache_entry
17361736
)
17371737

1738-
txn.async_call_after(prefill)
1738+
def local_prefill() -> None:
1739+
for cache_entry in to_prefill:
1740+
self.store._get_event_cache.set_local(
1741+
(cache_entry.event.event_id,), cache_entry
1742+
)
1743+
1744+
# The order these are called here is not as important as knowing that after the
1745+
# transaction is finished, the async_call_after will run before the call_after.
1746+
txn.async_call_after(external_prefill)
1747+
txn.call_after(local_prefill)
17391748

17401749
def _store_redaction(self, txn: LoggingTransaction, event: EventBase) -> None:
17411750
assert event.redacts is not None

synapse/storage/databases/main/events_worker.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -883,7 +883,7 @@ def invalidate_get_event_cache_after_txn(
883883

884884
async def _invalidate_async_get_event_cache(self, event_id: str) -> None:
885885
"""
886-
Invalidates an event in the asyncronous get event cache, which may be remote.
886+
Invalidates an event in the asynchronous get event cache, which may be remote.
887887
888888
Arguments:
889889
event_id: the event ID to invalidate

synapse/util/caches/lrucache.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -842,7 +842,13 @@ def get_local(
842842
return self._lru_cache.get(key, update_metrics=update_metrics)
843843

844844
async def set(self, key: KT, value: VT) -> None:
845-
self._lru_cache.set(key, value)
845+
# This will add the entries in the correct order, local first external second
846+
self.set_local(key, value)
847+
await self.set_external(key, value)
848+
849+
async def set_external(self, key: KT, value: VT) -> None:
850+
# This method should add an entry to any configured external cache, in this case noop.
851+
pass
846852

847853
def set_local(self, key: KT, value: VT) -> None:
848854
self._lru_cache.set(key, value)

tests/storage/databases/main/test_events_worker.py

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,55 @@ def test_persisting_event_invalidates_cache(self) -> None:
139139
# That should result in a single db query to lookup
140140
self.assertEqual(ctx.get_resource_usage().db_txn_count, 1)
141141

142+
def test_persisting_event_prefills_get_event_cache(self) -> None:
143+
"""
144+
Test to make sure that the `_get_event_cache` is prefilled after we persist an
145+
event and returns the updated value.
146+
"""
147+
event, event_context = self.get_success(
148+
create_event(
149+
self.hs,
150+
room_id=self.room_id,
151+
sender=self.user,
152+
type="test_event_type",
153+
content={"body": "conflabulation"},
154+
)
155+
)
156+
157+
# First, check `_get_event_cache` for the event we just made
158+
# to verify it's not in the cache.
159+
res = self.store._get_event_cache.get_local((event.event_id,))
160+
self.assertEqual(res, None, "Event was cached when it should not have been.")
161+
162+
with LoggingContext(name="test") as ctx:
163+
# Persist the event which should invalidate then prefill the
164+
# `_get_event_cache` so we don't return stale values.
165+
# Side Note: Apparently, persisting an event isn't a transaction in the
166+
# sense that it is recorded in the LoggingContext
167+
persistence = self.hs.get_storage_controllers().persistence
168+
assert persistence is not None
169+
self.get_success(
170+
persistence.persist_event(
171+
event,
172+
event_context,
173+
)
174+
)
175+
176+
# Check `_get_event_cache` again and we should see the updated fact
177+
# that we now have the event cached after persisting it.
178+
res = self.store._get_event_cache.get_local((event.event_id,))
179+
self.assertEqual(res.event, event, "Event not cached as expected.") # type: ignore
180+
181+
# Try and fetch the event from the database.
182+
self.get_success(self.store.get_event(event.event_id))
183+
184+
# Verify that the database hit was avoided.
185+
self.assertEqual(
186+
ctx.get_resource_usage().evt_db_fetch_count,
187+
0,
188+
"Database was hit, which would not happen if event was cached.",
189+
)
190+
142191
def test_invalidate_cache_by_room_id(self) -> None:
143192
"""
144193
Test to make sure that all events associated with the given `(room_id,)`

0 commit comments

Comments
 (0)