Fix exceptions in logs when failing to get remote room list (#10541)

changelog.d/10541.bugfix

@@ -0,0 +1 @@
+Fix exceptions in logs when failing to get remote room list.

synapse/federation/federation_client.py

@@ -1108,7 +1108,8 @@ class FederationClient(FederationBase):
             The response from the remote server.
 
         Raises:
-            HttpResponseException: There was an exception returned from the remote server
+            HttpResponseException / RequestSendFailed: There was an exception
+                returned from the remote server
             SynapseException: M_FORBIDDEN when the remote server has disallowed publicRoom
                 requests over federation
 

synapse/handlers/room_list.py

@@ -356,6 +356,12 @@ class RoomListHandler(BaseHandler):
         include_all_networks: bool = False,
         third_party_instance_id: Optional[str] = None,
     ) -> JsonDict:
+        """Get the public room list from remote server
+
+        Raises:
+            SynapseError
+        """
+
         if not self.enable_room_list_search:
             return {"chunk": [], "total_room_count_estimate": 0}
 
@@ -395,13 +401,16 @@ class RoomListHandler(BaseHandler):
             limit = None
             since_token = None
 
-        res = await self._get_remote_list_cached(
+        try:
-            server_name,
+            res = await self._get_remote_list_cached(
-            limit=limit,
+                server_name,
-            since_token=since_token,
+                limit=limit,
-            include_all_networks=include_all_networks,
+                since_token=since_token,
-            third_party_instance_id=third_party_instance_id,
+                include_all_networks=include_all_networks,
-        )
+                third_party_instance_id=third_party_instance_id,
+            )
+        except (RequestSendFailed, HttpResponseException):
+            raise SynapseError(502, "Failed to fetch room list")
 
         if search_filter:
             res = {
@@ -423,20 +432,21 @@ class RoomListHandler(BaseHandler):
         include_all_networks: bool = False,
         third_party_instance_id: Optional[str] = None,
     ) -> JsonDict:
+        """Wrapper around FederationClient.get_public_rooms that caches the
+        result.
+        """
+
         repl_layer = self.hs.get_federation_client()
         if search_filter:
             # We can't cache when asking for search
-            try:
+            return await repl_layer.get_public_rooms(
-                return await repl_layer.get_public_rooms(
+                server_name,
-                    server_name,
+                limit=limit,
-                    limit=limit,
+                since_token=since_token,
-                    since_token=since_token,
+                search_filter=search_filter,
-                    search_filter=search_filter,
+                include_all_networks=include_all_networks,
-                    include_all_networks=include_all_networks,
+                third_party_instance_id=third_party_instance_id,
-                    third_party_instance_id=third_party_instance_id,
+            )
-                )
-            except (RequestSendFailed, HttpResponseException):
-                raise SynapseError(502, "Failed to fetch room list")
 
         key = (
             server_name,

synapse/rest/client/v1/room.py

@@ -23,7 +23,6 @@ from synapse.api.constants import EventContentFields, EventTypes, Membership
 from synapse.api.errors import (
     AuthError,
     Codes,
-    HttpResponseException,
     InvalidClientCredentialsError,
     ShadowBanError,
     SynapseError,
@@ -783,12 +782,9 @@ class PublicRoomListRestServlet(TransactionRestServlet):
                     Codes.INVALID_PARAM,
                 )
 
-            try:
+            data = await handler.get_remote_public_room_list(
-                data = await handler.get_remote_public_room_list(
+                server, limit=limit, since_token=since_token
-                    server, limit=limit, since_token=since_token
+            )
-                )
-            except HttpResponseException as e:
-                raise e.to_synapse_error()
         else:
             data = await handler.get_local_public_room_list(
                 limit=limit, since_token=since_token
@@ -836,17 +832,15 @@ class PublicRoomListRestServlet(TransactionRestServlet):
                     Codes.INVALID_PARAM,
                 )
 
-            try:
+            data = await handler.get_remote_public_room_list(
-                data = await handler.get_remote_public_room_list(
+                server,
-                    server,
+                limit=limit,
-                    limit=limit,
+                since_token=since_token,
-                    since_token=since_token,
+                search_filter=search_filter,
-                    search_filter=search_filter,
+                include_all_networks=include_all_networks,
-                    include_all_networks=include_all_networks,
+                third_party_instance_id=third_party_instance_id,
-                    third_party_instance_id=third_party_instance_id,
+            )
-                )
+
-            except HttpResponseException as e:
-                raise e.to_synapse_error()
         else:
             data = await handler.get_local_public_room_list(
                 limit=limit,

tests/rest/client/v1/test_rooms.py

@@ -19,11 +19,14 @@
 
 import json
 from typing import Iterable
-from unittest.mock import Mock
+from unittest.mock import Mock, call
 from urllib import parse as urlparse
 
+from twisted.internet import defer
+
 import synapse.rest.admin
 from synapse.api.constants import EventContentFields, EventTypes, Membership
+from synapse.api.errors import HttpResponseException
 from synapse.handlers.pagination import PurgeStatus
 from synapse.rest import admin
 from synapse.rest.client.v1 import directory, login, profile, room
@@ -1124,6 +1127,93 @@ class PublicRoomsRestrictedTestCase(unittest.HomeserverTestCase):
         self.assertEqual(channel.code, 200, channel.result)
 
 
+class PublicRoomsTestRemoteSearchFallbackTestCase(unittest.HomeserverTestCase):
+    """Test that we correctly fallback to local filtering if a remote server
+    doesn't support search.
+    """
+
+    servlets = [
+        synapse.rest.admin.register_servlets_for_client_rest_resource,
+        room.register_servlets,
+        login.register_servlets,
+    ]
+
+    def make_homeserver(self, reactor, clock):
+        return self.setup_test_homeserver(federation_client=Mock())
+
+    def prepare(self, reactor, clock, hs):
+        self.register_user("user", "pass")
+        self.token = self.login("user", "pass")
+
+        self.federation_client = hs.get_federation_client()
+
+    def test_simple(self):
+        "Simple test for searching rooms over federation"
+        self.federation_client.get_public_rooms.side_effect = (
+            lambda *a, **k: defer.succeed({})
+        )
+
+        search_filter = {"generic_search_term": "foobar"}
+
+        channel = self.make_request(
+            "POST",
+            b"/_matrix/client/r0/publicRooms?server=testserv",
+            content={"filter": search_filter},
+            access_token=self.token,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+        self.federation_client.get_public_rooms.assert_called_once_with(
+            "testserv",
+            limit=100,
+            since_token=None,
+            search_filter=search_filter,
+            include_all_networks=False,
+            third_party_instance_id=None,
+        )
+
+    def test_fallback(self):
+        "Test that searching public rooms over federation falls back if it gets a 404"
+
+        # The `get_public_rooms` should be called again if the first call fails
+        # with a 404, when using search filters.
+        self.federation_client.get_public_rooms.side_effect = (
+            HttpResponseException(404, "Not Found", b""),
+            defer.succeed({}),
+        )
+
+        search_filter = {"generic_search_term": "foobar"}
+
+        channel = self.make_request(
+            "POST",
+            b"/_matrix/client/r0/publicRooms?server=testserv",
+            content={"filter": search_filter},
+            access_token=self.token,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+        self.federation_client.get_public_rooms.assert_has_calls(
+            [
+                call(
+                    "testserv",
+                    limit=100,
+                    since_token=None,
+                    search_filter=search_filter,
+                    include_all_networks=False,
+                    third_party_instance_id=None,
+                ),
+                call(
+                    "testserv",
+                    limit=None,
+                    since_token=None,
+                    search_filter=None,
+                    include_all_networks=False,
+                    third_party_instance_id=None,
+                ),
+            ]
+        )
+
+
 class PerRoomProfilesForbiddenTestCase(unittest.HomeserverTestCase):
 
     servlets = [