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

Faster joins: use initial list of servers if we don't have the full state yet #14408

Merged
merged 8 commits into from
Nov 24, 2022
Merged

Faster joins: use initial list of servers if we don't have the full state yet #14408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8cc46ad
Use initial list of servers if we don't have the full state yet
Nov 10, 2022
a9b2184
Update synapse/federation/sender/__init__.py
Nov 14, 2022
25cec00
Update changelog.d/14408.misc
Nov 14, 2022
bb99729
Avoid race
Nov 14, 2022
169b47c
Merge branch 'develop' into mv/partial-join-fed-send
Nov 14, 2022
87d1c1d
Fix mypy
Nov 14, 2022
fd19046
Revert "Fix mypy"
Nov 14, 2022
1820e43
Merge branch 'develop' into mv/partial-join-fed-send
Nov 14, 2022
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/14408.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Faster joins: send events to initial list of servers if we don't have the full state yet.
18 changes: 17 additions & 1 deletion synapse/federation/sender/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,23 @@ async def handle_event(event: EventBase) -> None:
# If there are no prev event IDs then the state is empty
# and so no remote servers in the room
destinations = set()
else:

if destinations is None:
# During partial join we use the set of servers that we got
# when beginning the join. It's still possible that we send
# events to servers that left the room in the meantime, but
# we consider that an acceptable risk since it is only our own
# events that we leak and not other server's ones.
partial_state_destinations = (
await self.store.get_partial_state_servers_at_join(
event.room_id
)
)

if len(partial_state_destinations) > 0:
destinations = partial_state_destinations
Comment on lines +439 to +451
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realised that we won't send events to servers that join after us.

self._storage_controllers.state.get_current_hosts_in_room_or_partial_state_approximation(room_id) returns a list that does include those servers (apologies for the naming), but it never returns an empty list.
We may want to go back to the original structure (elif await self.store.is_partial_state_room(event.room_id):) but use that method instead.

Sorry for not catching this earlier!

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose there's a question here: how much are we happy to trust this approximation? In prior discussions I gathered that we were just going to stick with the original list handed out over /send_join — is that no longer the case?
I guess the client will see the memberships for the 'extras', even if we end up rejecting them later on, it won't ever send events to servers that the user couldn't see, so maybe it's OK?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing / have forgotten context from prior discussions. If we've decided to go with only the original list, then please ignore the comment!

It's hard to say what memberships an end user might actually see, given that they'll be doing lazy loading syncs, which only provide memberships when a user sends a message. I think the member list in Element Web might not work until the room is fully synced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't merged that yet because I am unsure.

To my understanding, the approximation is initial list + updates from membership events that we received after.
There is a possibility that we can't validate those events out of missing state, hence I think we probably want to not use the approximation ?
We also can't be sure of the initial servers_in_room but at least we trust only one server there.

In the grand scheme of things it's not a big trouble because we potentially only leak our own events.

For now I am leaning towards merging it like that.


if destinations is None:
# We check the external cache for the destinations, which is
# stored per state group.

Expand Down