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

Faster joins: Refactor handling of servers in room #14954

Merged
merged 4 commits into from
Feb 3, 2023
Merged

Faster joins: Refactor handling of servers in room #14954

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
07853dd
Ensure list of servers in partial state room contains original server
Jan 31, 2023
2ec2796
Refactor `get_partial_state_servers_at_join`
Jan 31, 2023
f65d213
Add newsfile
Jan 31, 2023
163c68c
convert list of servers into a set
Feb 2, 2023
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
28 changes: 18 additions & 10 deletions synapse/federation/federation_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ class SendJoinResult:
# True if 'state' elides non-critical membership events
partial_state: bool

# if 'partial_state' is set, a list of the servers in the room (otherwise empty)
# If 'partial_state' is set, a list of the servers in the room (otherwise empty).
# Always contains the server we joined off.
servers_in_room: List[str]


Expand Down Expand Up @@ -1152,23 +1153,30 @@ async def _execute(pdu: EventBase) -> None:
% (auth_chain_create_events,)
)

if response.members_omitted and not response.servers_in_room:
raise InvalidResponseError(
"members_omitted was set, but no servers were listed in the room"
)
servers_in_room = response.servers_in_room
if response.members_omitted:
if not servers_in_room:
raise InvalidResponseError(
"members_omitted was set, but no servers were listed in the room"
)

if response.members_omitted and not partial_state:
raise InvalidResponseError(
"members_omitted was set, but we asked for full state"
)
if destination not in servers_in_room:
# `servers_in_room` is supposed to be a complete list.
# Fix things up if the remote homeserver is badly behaved.
servers_in_room = [destination] + servers_in_room
Copy link
Member

Choose a reason for hiding this comment

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

Should we be making this a set? What if the remote server returns the same thing for servers_in_room 100 times?

Is it possible for destination to no longer be in the room at this point? (I'm guessing no because we just got a response from it?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. We'll fail to insert the list into the database, because there's a unique constraint. And the join will fail.

There's nothing stopping destination from leaving the room immediately after our join really.
If destination leaves the room, we'll try syncing state from other servers in the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Representing the server list as a set sounds like a good idea, so I did it in 163c68c.


if not partial_state:
raise InvalidResponseError(
"members_omitted was set, but we asked for full state"
)
Comment on lines +1167 to +1170
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend moving this above the destination check to have all the error checking up-front.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 163c68c.


return SendJoinResult(
event=event,
state=signed_state,
auth_chain=signed_auth,
origin=destination,
partial_state=response.members_omitted,
servers_in_room=response.servers_in_room or [],
servers_in_room=servers_in_room or [],
)

# MSC3083 defines additional error codes for room joins.
Expand Down
4 changes: 3 additions & 1 deletion synapse/storage/databases/main/room.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1956,11 +1956,13 @@ async def store_partial_state_room(

Args:
room_id: the ID of the room
servers: other servers known to be in the room
servers: other servers known to be in the room. must include `joined_via`.
device_lists_stream_id: the device_lists stream ID at the time when we first
joined the room.
joined_via: the server name we requested a partial join from.
"""
assert joined_via in servers

await self.db_pool.runInteraction(
"store_partial_state_room",
self._store_partial_state_room_txn,
Expand Down