Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.
Merged
Changes from 1 commit
Commits
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
45 changes: 34 additions & 11 deletions synapse/federation/federation_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,7 @@ async def _try_destination_list(
description: str,
destinations: Iterable[str],
callback: Callable[[str], Awaitable[T]],
failover_on_unknown_endpoint: bool = False,
) -> T:
"""Try an operation on a series of servers, until it succeeds

Expand All @@ -476,6 +477,10 @@ async def _try_destination_list(
next server tried. Normally the stacktrace is logged but this is
suppressed if the exception is an InvalidResponseError.

failover_on_unknown_endpoint: if True, we will try other servers if it looks
like a server doesn't support the endpoint. This is typically useful
if the endpoint in question is new or experimental.

Returns:
The result of callback, if it succeeds

Expand All @@ -495,16 +500,31 @@ async def _try_destination_list(
except UnsupportedRoomVersionError:
raise
except HttpResponseException as e:
if not 500 <= e.code < 600:
raise e.to_synapse_error()
else:
logger.warning(
"Failed to %s via %s: %i %s",
description,
destination,
e.code,
e.args[0],
)
synapse_error = e.to_synapse_error()
failover = False

if 500 <= e.code < 600:
failover = True

elif failover_on_unknown_endpoint:
# there is no good way to detect an "unknown" endpoint. Dendrite
# returns a 404 (with no body); synapse returns a 400
# with M_UNRECOGNISED.
if e.code == 404 or (
e.code == 400 and synapse_error.errcode == Codes.UNRECOGNIZED
):
failover = True

if not failover:
Copy link
Member

Choose a reason for hiding this comment

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

Should this be if failover? I'm having some trouble following this logic. Previously the 500 error would raise a SynapseError, but now it won't (since failover is True in that case).

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously the 500 error would raise a SynapseError

I don't think that's right. Previously anything except a 500 error would raise a SynapseError.

I agree the logic here isn't super easy to follow. I'm very open to suggestions for how to reorganise it to make it clearer.

I was actually contemplating this logic in the shower this morning... Really, there ought to be a defined list of error codes that a well-behaved server can return for each endpoint - and anything else constitutes a misbehaving server whose response should be discounted. The trouble is that implementing that means getting into the weeds with refactoring the other code that uses _try_destination_list.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah I had missed the not in front of the first clause. 😢

I think the newer version is a bit clearer with the failover variable. I was just failing to see the equivalence to the old one.

I don't have any suggestions of how to make it better now. 👍

raise synapse_error from e

logger.warning(
"Failed to %s via %s: %i %s",
description,
destination,
e.code,
e.args[0],
)
except Exception:
logger.warning(
"Failed to %s via %s", description, destination, exc_info=True
Expand Down Expand Up @@ -1068,7 +1088,10 @@ async def send_request(destination: str) -> FederationSpaceSummaryResult:
raise InvalidResponseError(str(e))

return await self._try_destination_list(
"fetch space summary", destinations, send_request
"fetch space summary",
destinations,
send_request,
failover_on_unknown_endpoint=True,
)


Expand Down