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

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

Merged
merged 4 commits into from
Aug 6, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
79a5216
Fix exceptions in logs when failing to get remote room list
erikjohnston Aug 5, 2021
4f65b5d
Newsfile
erikjohnston Aug 5, 2021
02a99d6
Add sanity to exception handling
erikjohnston Aug 6, 2021
fb4160c
Add test
erikjohnston Aug 6, 2021
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/10541.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix exceptions in logs when failing to get remote room list.
20 changes: 12 additions & 8 deletions synapse/federation/federation_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
Codes,
FederationDeniedError,
HttpResponseException,
RequestSendFailed,
SynapseError,
UnsupportedRoomVersionError,
)
Expand Down Expand Up @@ -1113,14 +1114,17 @@ async def get_public_rooms(
requests over federation

"""
return await self.transport_layer.get_public_rooms(
remote_server,
limit,
since_token,
search_filter,
include_all_networks=include_all_networks,
third_party_instance_id=third_party_instance_id,
)
try:
return await self.transport_layer.get_public_rooms(
remote_server,
limit,
since_token,
search_filter,
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")
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to log the error as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll have already logged that the federation request has failed, so I don't think there's much point

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be picking error codes here: that should be handled in the REST server code, not the federation client code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeees, though what are you envisaging exactly? We can add an exception type for CouldNotTalkToRemote, but would that extend SynapseError (which is what we usually do), or handled explicitly at the rest layer, or the servlet layer?

Or would it be easier to hoist this back up into the room list handler again?

Copy link
Member

Choose a reason for hiding this comment

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

Right. I notice that get_remote_public_room_list has yet another layer of exception handling:

synapse/synapse/handlers/room_list.py

Lines 378 to 390 in 4f65b5d

except HttpResponseException as hre:
syn_err = hre.to_synapse_error()
if hre.code in (404, 405) or syn_err.errcode in (
Codes.UNRECOGNIZED,
Codes.NOT_FOUND,
):
logger.debug("Falling back to locally-filtered /publicRooms")
else:
# Not an error that should trigger a fallback.
raise SynapseError(502, "Failed to fetch room list")
except RequestSendFailed:
# Not an error that should trigger a fallback.
raise SynapseError(502, "Failed to fetch room list")
. Indeed, it looks like #10400 might have messed this up.

And, if that weren't enough of a mess, PublicRoomListRestServlet has another round of catching HttpResponseException, which I think is meant to be unreachable, but who the hell knows.

I think you should allow the RequestSendFailed / HttpResponseExceptions to propagate into RoomListHandler and raise an appropriate SynapseError there; and remove the handling from PublicRoomListRestServlet. And, for the love of $deity, document the behaviour of these things.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wheeeeeee untested features


async def get_missing_events(
self,
Expand Down
19 changes: 8 additions & 11 deletions synapse/handlers/room_list.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,17 +426,14 @@ async def _get_remote_list_cached(
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(
server_name,
limit=limit,
since_token=since_token,
search_filter=search_filter,
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")
return await repl_layer.get_public_rooms(
server_name,
limit=limit,
since_token=since_token,
search_filter=search_filter,
include_all_networks=include_all_networks,
third_party_instance_id=third_party_instance_id,
)

key = (
server_name,
Expand Down