From 5f84545b989b93712579f5151974ac3e619d88e1 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 Aug 2021 13:21:22 -0400 Subject: [PATCH 1/5] Reverse conditional to raise early. --- synapse/federation/federation_client.py | 79 +++++++++++++------------ 1 file changed, 40 insertions(+), 39 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 29979414e3d7..53ad7b3a1a4f 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -1372,51 +1372,52 @@ async def send_request( failover_on_unknown_endpoint=True, ) except SynapseError as e: + # If an unexpected error occurred, re-raise it. + if e.code != 502: + raise + # Fallback to the old federation API and translate the results if # no servers implement the new API. # # The algorithm below is a bit inefficient as it only attempts to - # get information for the requested room, but the legacy API may + # parse information for the requested room, but the legacy API may # return additional layers. - if e.code == 502: - legacy_result = await self.get_space_summary( - destinations, - room_id, - suggested_only, - max_rooms_per_space=None, - exclude_rooms=[], - ) + legacy_result = await self.get_space_summary( + destinations, + room_id, + suggested_only, + max_rooms_per_space=None, + exclude_rooms=[], + ) - # Find the requested room in the response (and remove it). - for _i, room in enumerate(legacy_result.rooms): - if room.get("room_id") == room_id: - break - else: - # The requested room was not returned, nothing we can do. - raise - requested_room = legacy_result.rooms.pop(_i) - - # Find any children events of the requested room. - children_events = [] - children_room_ids = set() - for event in legacy_result.events: - if event.room_id == room_id: - children_events.append(event.data) - children_room_ids.add(event.state_key) - # And add them under the requested room. - requested_room["children_state"] = children_events - - # Find the children rooms. - children = [] - for room in legacy_result.rooms: - if room.get("room_id") in children_room_ids: - children.append(room) - - # It isn't clear from the response whether some of the rooms are - # not accessible. - return requested_room, children, () - - raise + # Find the requested room in the response (and remove it). + for _i, room in enumerate(legacy_result.rooms): + if room.get("room_id") == room_id: + break + else: + # The requested room was not returned, nothing we can do. + raise + requested_room = legacy_result.rooms.pop(_i) + + # Find any children events of the requested room. + children_events = [] + children_room_ids = set() + for event in legacy_result.events: + if event.room_id == room_id: + children_events.append(event.data) + children_room_ids.add(event.state_key) + # And add them under the requested room. + requested_room["children_state"] = children_events + + # Find the children rooms. + children = [] + for room in legacy_result.rooms: + if room.get("room_id") in children_room_ids: + children.append(room) + + # It isn't clear from the response whether some of the rooms are + # not accessible. + return requested_room, children, () @attr.s(frozen=True, slots=True, auto_attribs=True) From 2bca5fac534f670b68890ad87cae7b5efa74ad61 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 Aug 2021 13:22:06 -0400 Subject: [PATCH 2/5] Cache the result of fetching the room hierarchy over federation. --- synapse/federation/federation_client.py | 29 +++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 53ad7b3a1a4f..8ab0ab1c9d36 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -110,6 +110,23 @@ def __init__(self, hs: "HomeServer"): reset_expiry_on_get=False, ) + # A cache for fetching the room hierarchy over federation. + # + # Some stale data over federation is OK, but must be refreshed + # periodically since the local server is in the room. + # + # It is a map of (room ID, suggested-only) -> the response of + # get_room_hierarchy. + self._get_room_hierarchy_cache: ExpiringCache[ + Tuple[str, bool], Tuple[JsonDict, Sequence[JsonDict], Sequence[str]] + ] = ExpiringCache( + cache_name="get_room_hierarchy_cache", + clock=self._clock, + max_len=1000, + expiry_ms=5 * 60 * 1000, + reset_expiry_on_get=False, + ) + def _clear_tried_cache(self): """Clear pdu_destination_tried cache""" now = self._clock.time_msec() @@ -1319,6 +1336,10 @@ async def get_room_hierarchy( remote servers """ + cached_result = self._get_room_hierarchy_cache.get((room_id, suggested_only)) + if cached_result: + return cached_result + async def send_request( destination: str, ) -> Tuple[JsonDict, Sequence[JsonDict], Sequence[str]]: @@ -1365,7 +1386,7 @@ async def send_request( return room, children, inaccessible_children try: - return await self._try_destination_list( + result = await self._try_destination_list( "fetch room hierarchy", destinations, send_request, @@ -1417,7 +1438,11 @@ async def send_request( # It isn't clear from the response whether some of the rooms are # not accessible. - return requested_room, children, () + result = (requested_room, children, ()) + + # Cache the result to avoid fetching data over federation every time. + self._get_room_hierarchy_cache[(room_id, suggested_only)] = result + return result @attr.s(frozen=True, slots=True, auto_attribs=True) From 0552cb358748399228b22faba7eeac513fc91f1e Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 18 Aug 2021 13:40:46 -0400 Subject: [PATCH 3/5] Newsfragment --- changelog.d/10647.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10647.feature diff --git a/changelog.d/10647.feature b/changelog.d/10647.feature new file mode 100644 index 000000000000..3a207d056df0 --- /dev/null +++ b/changelog.d/10647.feature @@ -0,0 +1 @@ +Improve the performance of the `/hiearchy` API (from [MSC2946](https://github.com/matrix-org/matrix-doc/pull/2946)) by caching responses received over federation. From 99d0b48a98793759712d82955d19814cb218e2b9 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 25 Aug 2021 12:16:05 -0400 Subject: [PATCH 4/5] Fix typo. Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- changelog.d/10647.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/10647.feature b/changelog.d/10647.feature index 3a207d056df0..4407a9030d55 100644 --- a/changelog.d/10647.feature +++ b/changelog.d/10647.feature @@ -1 +1 @@ -Improve the performance of the `/hiearchy` API (from [MSC2946](https://github.com/matrix-org/matrix-doc/pull/2946)) by caching responses received over federation. +Improve the performance of the `/hierarchy` API (from [MSC2946](https://github.com/matrix-org/matrix-doc/pull/2946)) by caching responses received over federation. From f35681f70deb83ee43f48655346ee369c7bb9d79 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 25 Aug 2021 12:16:40 -0400 Subject: [PATCH 5/5] feature -> misc --- changelog.d/{10647.feature => 10647.misc} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{10647.feature => 10647.misc} (100%) diff --git a/changelog.d/10647.feature b/changelog.d/10647.misc similarity index 100% rename from changelog.d/10647.feature rename to changelog.d/10647.misc