-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Check appservice user interest against the local users instead of all users (get_users_in_room
mis-use)
#13958
Changes from 5 commits
99a623d
806b255
5f0f815
a8be41b
72985df
92b9da2
5d3c6a3
76435c7
4451998
1218f03
7bd3803
33f718c
cf8299b
3de90e6
ab33cd6
3223512
d913ceb
4f29e75
2665aa0
39e2ead
33a5b70
92400ff
426ef5c
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 @@ | ||
Fix performance bottleneck with heavy appservice and bridged users in Synapse by checking appservice user interest against the local users instead of all users. | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -172,12 +172,23 @@ async def _matches_user_in_member_list( | |
Returns: | ||
True if this service would like to know about this room. | ||
""" | ||
member_list = await store.get_users_in_room( | ||
# We can use `get_local_users_in_room(...)` here because an application | ||
# service can only act on behalf of users of the server it's on. | ||
# | ||
# In the future, we can consider re-using | ||
# `store.get_app_service_users_in_room` which is very similar to this | ||
# function but has a slightly worse performance than this because we | ||
# have an early escape-hatch if we find a single user that the | ||
# appservice is interested in. The juice would be worth the squeeze if | ||
# `store.get_app_service_users_in_room` was used in more places besides | ||
# an experimental MSC. But for now we can avoid doing more work and | ||
anoadragon453 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# barely using it later. | ||
Comment on lines
+179
to
+186
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. For more context on this comment, see #13958 (comment) |
||
local_user_ids = await store.get_local_users_in_room( | ||
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. This changes the behavior of 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 see how an appservice could control remote users, so I think so...but might be worth asking someone with more info? 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 not really a question of controlling the users, but more of whether the application service wants to receive events and updates related to those users, which an application service may well want to do for events coming from non-local users. So I think this is a change we wouldn't want to go ahead with. 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. Interesting! I hadn't considered this use case. For my own reference, the spec doesn't mention any restriction either: https://spec.matrix.org/v1.1/application-service-api/#registration I've updated the comments to clarify local and remote so it's obvious if this is attempted again. We do have tests around this kinda but because they were just mocking 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. Created #14000 to introduce these clarification changes with so we can leave the diff in this PR for reference of what we didn't want to do. 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. Quite possibly, though the historical context (which may be lost to time) is still very prevelant here. 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.
Is there something specific to point to here? Or just from your memory? 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. The implied parts of "r0 was a nightmare" above :p (mostly my memory, but also the memory of others from around that time) 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. Created a MSC for this: MSC3905 And prodding this point in case we can merge earlier,
I tend to agree with @anoadragon453 here (I think we agree 😬). The use cases of seeing events from remote users (like moderation) is already supported by matching the entire room with the We have to rip the band-aid off at some point whether it before or after matrix-org/matrix-spec-proposals#3905 merges. Maybe there is a way to detect people relying on the old behavior? -> #13958 (comment) 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. MSC3905 is merged and this behavior is ok to change now ✅ |
||
room_id, on_invalidate=cache_context.invalidate | ||
) | ||
|
||
# check joined member events | ||
for user_id in member_list: | ||
for user_id in local_user_ids: | ||
if self.is_interested_in_user(user_id): | ||
return True | ||
return False | ||
|
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.
For anyone curious, there might be a few more
get_users_in_room
mis-uses in thesync
andpresence
code for someone who has more domain knowledge on what those things should exactly do.For example #13967 (feel free to pick it up)