-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Implement MSC3706: partial state in /send_join
response
#11967
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Experimental implementation of [MSC3706](https://github.com/matrix-org/matrix-doc/pull/3706): extensions to `/send_join` to support reduced response size. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
Any, | ||
Awaitable, | ||
Callable, | ||
Collection, | ||
Dict, | ||
Iterable, | ||
List, | ||
|
@@ -64,7 +65,7 @@ | |
ReplicationGetQueryRestServlet, | ||
) | ||
from synapse.storage.databases.main.lock import Lock | ||
from synapse.types import JsonDict, get_domain_from_id | ||
from synapse.types import JsonDict, StateMap, get_domain_from_id | ||
from synapse.util import json_decoder, unwrapFirstError | ||
from synapse.util.async_helpers import Linearizer, concurrently_execute, gather_results | ||
from synapse.util.caches.response_cache import ResponseCache | ||
|
@@ -571,7 +572,7 @@ async def _on_state_ids_request_compute( | |
) -> JsonDict: | ||
state_ids = await self.handler.get_state_ids_for_pdu(room_id, event_id) | ||
auth_chain_ids = await self.store.get_auth_chain_ids(room_id, state_ids) | ||
return {"pdu_ids": state_ids, "auth_chain_ids": auth_chain_ids} | ||
return {"pdu_ids": state_ids, "auth_chain_ids": list(auth_chain_ids)} | ||
|
||
async def _on_context_state_request_compute( | ||
self, room_id: str, event_id: Optional[str] | ||
|
@@ -645,27 +646,61 @@ async def on_invite_request( | |
return {"event": ret_pdu.get_pdu_json(time_now)} | ||
|
||
async def on_send_join_request( | ||
self, origin: str, content: JsonDict, room_id: str | ||
self, | ||
origin: str, | ||
content: JsonDict, | ||
room_id: str, | ||
caller_supports_partial_state: bool = False, | ||
) -> Dict[str, Any]: | ||
event, context = await self._on_send_membership_event( | ||
origin, content, Membership.JOIN, room_id | ||
) | ||
|
||
prev_state_ids = await context.get_prev_state_ids() | ||
state_ids = list(prev_state_ids.values()) | ||
auth_chain = await self.store.get_auth_chain(room_id, state_ids) | ||
state = await self.store.get_events(state_ids) | ||
|
||
state_event_ids: Collection[str] | ||
servers_in_room: Optional[Collection[str]] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are basically so that the joining homeserver has some other homeservers to contact to fetch events if we were to just vanish, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yeah, this is the main driver right now. |
||
if caller_supports_partial_state: | ||
state_event_ids = _get_event_ids_for_partial_state_join( | ||
event, prev_state_ids | ||
) | ||
servers_in_room = await self.state.get_hosts_in_room_at_events( | ||
room_id, event_ids=event.prev_event_ids() | ||
) | ||
else: | ||
state_event_ids = prev_state_ids.values() | ||
servers_in_room = None | ||
|
||
auth_chain_event_ids = await self.store.get_auth_chain_ids( | ||
room_id, state_event_ids | ||
) | ||
Comment on lines
+674
to
+676
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've split the previous call to |
||
|
||
# if the caller has opted in, we can omit any auth_chain events which are | ||
# already in state_event_ids | ||
if caller_supports_partial_state: | ||
auth_chain_event_ids.difference_update(state_event_ids) | ||
|
||
auth_chain_events = await self.store.get_events_as_list(auth_chain_event_ids) | ||
state_events = await self.store.get_events_as_list(state_event_ids) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
# we try to do all the async stuff before this point, so that time_now is as | ||
# accurate as possible. | ||
time_now = self._clock.time_msec() | ||
event_json = event.get_pdu_json() | ||
return { | ||
event_json = event.get_pdu_json(time_now) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. may as well fix this to use the same "time_now" timestamp as |
||
resp = { | ||
# TODO Remove the unstable prefix when servers have updated. | ||
"org.matrix.msc3083.v2.event": event_json, | ||
"event": event_json, | ||
"state": [p.get_pdu_json(time_now) for p in state.values()], | ||
"auth_chain": [p.get_pdu_json(time_now) for p in auth_chain], | ||
"state": [p.get_pdu_json(time_now) for p in state_events], | ||
"auth_chain": [p.get_pdu_json(time_now) for p in auth_chain_events], | ||
"org.matrix.msc3706.partial_state": caller_supports_partial_state, | ||
} | ||
|
||
if servers_in_room is not None: | ||
resp["org.matrix.msc3706.servers_in_room"] = list(servers_in_room) | ||
|
||
return resp | ||
|
||
async def on_make_leave_request( | ||
self, origin: str, room_id: str, user_id: str | ||
) -> Dict[str, Any]: | ||
|
@@ -1339,3 +1374,39 @@ async def on_query(self, query_type: str, args: dict) -> JsonDict: | |
# error. | ||
logger.warning("No handler registered for query type %s", query_type) | ||
raise NotFoundError("No handler for Query type '%s'" % (query_type,)) | ||
|
||
|
||
def _get_event_ids_for_partial_state_join( | ||
join_event: EventBase, | ||
prev_state_ids: StateMap[str], | ||
) -> Collection[str]: | ||
"""Calculate state to be retuned in a partial_state send_join | ||
|
||
Args: | ||
join_event: the join event being send_joined | ||
prev_state_ids: the event ids of the state before the join | ||
|
||
Returns: | ||
the event ids to be returned | ||
""" | ||
|
||
# return all non-member events | ||
state_event_ids = { | ||
event_id | ||
for (event_type, state_key), event_id in prev_state_ids.items() | ||
if event_type != EventTypes.Member | ||
} | ||
|
||
# we also need the current state of the current user (it's going to | ||
# be an auth event for the new join, so we may as well return it) | ||
current_membership_event_id = prev_state_ids.get( | ||
(EventTypes.Member, join_event.state_key) | ||
) | ||
if current_membership_event_id is not None: | ||
state_event_ids.add(current_membership_event_id) | ||
|
||
# TODO: return a few more members: | ||
# - those with invites | ||
# - those that are kicked? / banned | ||
Comment on lines
+1408
to
+1410
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interested in more rationale/elaboration here, if you have any more thoughts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, MSC2775 lists a whole bunch of members we might want to return. I'm unconvinced by some of them, so this is here just as a thing to return to. |
||
|
||
return state_event_ids |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -412,6 +412,16 @@ class FederationV2SendJoinServlet(BaseFederationServerServlet): | |
|
||
PREFIX = FEDERATION_V2_PREFIX | ||
|
||
def __init__( | ||
self, | ||
hs: "HomeServer", | ||
authenticator: Authenticator, | ||
ratelimiter: FederationRateLimiter, | ||
server_name: str, | ||
): | ||
super().__init__(hs, authenticator, ratelimiter, server_name) | ||
self._msc3706_enabled = hs.config.experimental.msc3706_enabled | ||
|
||
async def on_PUT( | ||
self, | ||
origin: str, | ||
|
@@ -422,7 +432,15 @@ async def on_PUT( | |
) -> Tuple[int, JsonDict]: | ||
# TODO(paul): assert that event_id parsed from path actually | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't even know who Paul is! :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's @leonerd :). He did some contracting for us back in the pre-NVL days. Which tells us how long this particular TODO has gone undone :'( |
||
# match those given in content | ||
result = await self.handler.on_send_join_request(origin, content, room_id) | ||
|
||
partial_state = False | ||
if self._msc3706_enabled: | ||
partial_state = parse_boolean_from_args( | ||
query, "org.matrix.msc3706.partial_state", default=False | ||
) | ||
result = await self.handler.on_send_join_request( | ||
origin, content, room_id, caller_supports_partial_state=partial_state | ||
) | ||
return 200, result | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really sure what the point of the
list()
was here. It might even be a carry-over from the py2->py3 conversion, where the assumption was that you had to wrap any calls to.keys()
or.values()
inlist()
unless you knew otherwise. Anyway, I've removed it while I'm in the area.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems completely reasonable (you can iterate over them multiple times, they have the same truthiness rules as lists, and so on) — I would probably agree with you about the Python 2 suspicion, since I think it behaved quite a bit differently back then.