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

Make get_room_version use cached get_room_version_id. #11808

Merged
merged 7 commits into from
Mar 2, 2022
Merged

Make get_room_version use cached get_room_version_id. #11808

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ea23217
let get_room_version use cached get_room_version_id
lukasdenk Jan 3, 2022
397459e
Add Changelog
lukasdenk Jan 24, 2022
1852397
Add "await" to method call
lukasdenk Jan 24, 2022
73ffce1
rename private method to 1 preceeding "_"
lukasdenk Jan 26, 2022
3ca4687
invalidate cache in SpaceSummaryTestCase.test_unknown_room_version
lukasdenk Jan 26, 2022
2d33cbc
convert method to function
lukasdenk Feb 8, 2022
8b63570
add comment to explain cache invalidation in test
lukasdenk Feb 8, 2022
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/11808.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make method `get_room_version` use cached `get_room_version_id`.
14 changes: 7 additions & 7 deletions synapse/storage/databases/main/state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,8 @@ async def get_room_version(self, room_id: str) -> RoomVersion:
Typically this happens if support for the room's version has been
removed from Synapse.
"""
return await self.db_pool.runInteraction(
"get_room_version_txn",
self.get_room_version_txn,
room_id,
)
room_version_id = await self.get_room_version_id(room_id)
return self._retrieve_and_check_room_version(room_id, room_version_id)

def get_room_version_txn(
self, txn: LoggingTransaction, room_id: str
Expand All @@ -82,14 +79,17 @@ def get_room_version_txn(
removed from Synapse.
"""
room_version_id = self.get_room_version_id_txn(txn, room_id)
v = KNOWN_ROOM_VERSIONS.get(room_version_id)
return self._retrieve_and_check_room_version(room_id, room_version_id)

def _retrieve_and_check_room_version(
lukasdenk marked this conversation as resolved.
Show resolved Hide resolved
self, room_id: str, room_version_id: str
) -> RoomVersion:
v = KNOWN_ROOM_VERSIONS.get(room_version_id)
if not v:
raise UnsupportedRoomVersionError(
"Room %s uses a room version %s which is no longer supported"
% (room_id, room_version_id)
)

return v

@cached(max_entries=10000)
Expand Down
1 change: 1 addition & 0 deletions tests/handlers/test_room_summary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,6 +727,7 @@ def test_unknown_room_version(self):
desc="updated-room-version",
)
)
self.hs.get_datastores().main.get_room_version_id.invalidate((self.room,))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

It's bad practice to add invalidations for the tests since it might cover up an invalidation problem in the real code (I ran into this too during one of my recent PRs).
It's not clear to my why it's needed in the first place, but ideally we should fix it so it's not needed, or failing that, add a comment about it?

Copy link
Contributor Author

@lukasdenk lukasdenk Feb 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The test tests whether a room summary still succeeds, even if the room's version is invalid. To do so, it sets the room version id to unknown-room-version in the lines above. However the room version has already been requested before. Therefore, get_room_version will not return unknown-room-version as the room version for room_id. Hence, get_room_version's cache must be invalidated.

I actually included a comment now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is weird though, is that removing these lines

synapse/tests/handlers/test_room_summary.py

Lines 722 to 729 in 3655585

self.get_success(
self.hs.get_datastores().main.db_pool.simple_update(
"rooms",
keyvalues={"room_id": self.room},
updatevalues={"room_version": "unknown-room-version"},
desc="updated-room-version",
)
)

(and not invalidating get_version_id) results in a different exception than including the above lines (and not invalidating get_version_id). However, in both cases, get_version_id returns the same, namely '6'. However, I would expect both scenarios would result in the same exception. @reivilibre Do you have any idea why this could be the case? Should I dig more deeply into the reasons?

The first approach fails here

synapse/tests/handlers/test_room_summary.py

Line 734 in 3655585

self._assert_rooms(result, expected)

with

Failure
Traceback (most recent call last):
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 478, in addCallbacks
    self._runCallbacks()
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 858, in _runCallbacks
    current.result = callback(  # type: ignore[misc]
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_asynctest.py", line 152, in deferTestMethod
    d = self._run(self._testMethodName, result)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_asynctest.py", line 122, in _run
    d = defer.maybeDeferred(
--- <exception caught here> ---
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 191, in maybeDeferred
    result = f(*args, **kwargs)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/utils.py", line 209, in runWithWarningsSuppressed
    raise exc_info[1].with_traceback(exc_info[2])
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/utils.py", line 205, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/home/lukas/PycharmProjects/synapse/tests/handlers/test_room_summary.py", line 737, in test_unknown_room_version
    self._assert_rooms(result, expected)
  File "/home/lukas/PycharmProjects/synapse/tests/handlers/test_room_summary.py", line 178, in _assert_rooms
    self.assertCountEqual(
  File "/usr/lib/python3.8/unittest/case.py", line 1272, in assertCountEqual
    self.fail(msg)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_synctest.py", line 361, in fail
    raise self.failureException(msg)
twisted.trial.unittest.FailTest: Element counts were not equal:
First has 1, Second has 0:  '!XgELWsagGJJtucVyif:test'

while the latter fails here

synapse/tests/handlers/test_room_summary.py

Line 731 in 3655585

result = self.get_success(self.handler.get_space_summary(self.user, self.space))

with

Failure
Traceback (most recent call last):
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 478, in addCallbacks
    self._runCallbacks()
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 858, in _runCallbacks
    current.result = callback(  # type: ignore[misc]
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_asynctest.py", line 152, in deferTestMethod
    d = self._run(self._testMethodName, result)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_asynctest.py", line 122, in _run
    d = defer.maybeDeferred(
--- <exception caught here> ---
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 191, in maybeDeferred
    result = f(*args, **kwargs)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/utils.py", line 209, in runWithWarningsSuppressed
    raise exc_info[1].with_traceback(exc_info[2])
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/utils.py", line 205, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/home/lukas/PycharmProjects/synapse/tests/handlers/test_room_summary.py", line 734, in test_unknown_room_version
    result = self.get_success(self.handler.get_space_summary(self.user, self.space))
  File "/home/lukas/PycharmProjects/synapse/tests/unittest.py", line 522, in get_success
    return self.successResultOf(d)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_synctest.py", line 700, in successResultOf
    self.fail(
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/trial/_synctest.py", line 361, in fail
    raise self.failureException(msg)
twisted.trial.unittest.FailTest: Success result expected on <Deferred at 0x7f6e46cdd460 current result: None>, found failure result instead:
Traceback (most recent call last):
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 662, in callback
    self._startRunCallbacks(result)
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 764, in _startRunCallbacks
    self._runCallbacks()
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 858, in _runCallbacks
    current.result = callback(  # type: ignore[misc]
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 1751, in gotResult
    current_context.run(_inlineCallbacks, r, gen, status)
--- <exception caught here> ---
  File "/home/lukas/PycharmProjects/synapse/env/lib/python3.8/site-packages/twisted/internet/defer.py", line 1661, in _inlineCallbacks
    result = current_context.run(gen.send, result)
  File "/home/lukas/PycharmProjects/synapse/synapse/handlers/room_summary.py", line 176, in get_space_summary
    room_entry = await self._summarize_local_room(
  File "/home/lukas/PycharmProjects/synapse/synapse/handlers/room_summary.py", line 651, in _summarize_local_room
    if not await self._is_local_room_accessible(room_id, requester, origin):
  File "/home/lukas/PycharmProjects/synapse/synapse/handlers/room_summary.py", line 860, in _is_local_room_accessible
    join_rules_event = await self._store.get_event(join_rules_event_id)
  File "/home/lukas/PycharmProjects/synapse/synapse/storage/databases/main/events_worker.py", line 385, in get_event
    raise NotFoundError("Could not find event %s" % (event_id,))
synapse.api.errors.NotFoundError: 404: Could not find event $7nE7ON6WDexQYs-xMFG5SMOfIe2wx00BQPwgItkyyIg

Copy link
Contributor

Choose a reason for hiding this comment

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

synapse.api.errors.NotFoundError: 404: Could not find event $7nE7ON6WDexQYs-xMFG5SMOfIe2wx00BQPwgItkyyIg

Looking at this one, it seems like it occurs because the event fetching code fetches the room version separately and then encounters a room version it doesn't understand: it then skips fetching that event. (This is a design choice: events in unknown room versions are treated as nonexistent events because we don't know how to decode them)

I think that's fine; in practice the version of a room doesn't change and so there's no way the cache would get out of sync with the database.

twisted.trial.unittest.FailTest: Element counts were not equal: First has 1, Second has 0: '!XgELWsagGJJtucVyif:test'

This is happening because if you don't set the room version to something unknown, the code will (correctly) generate a room summary entry for it, which the test doesn't expect for unknown room versions.

I think your code is therefore fine as-is!


result = self.get_success(self.handler.get_space_summary(self.user, self.space))
# The result should have only the space, along with a link from space -> room.
Expand Down