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

Check appservice user interest against the local users instead of all users (get_users_in_room mis-use) #13958

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
99a623d
Check appservice user interest against the local users instead of all…
MadLittleMods Sep 29, 2022
806b255
Remove non-appservice usages split out to other PRs
MadLittleMods Sep 29, 2022
5f0f815
Add changelog
MadLittleMods Sep 29, 2022
a8be41b
Revert back to using our own `_matches_user_in_member_list` thing
MadLittleMods Sep 29, 2022
72985df
Rename variables to 'local' to be obvious our intention
MadLittleMods Sep 29, 2022
92b9da2
Fix tests
MadLittleMods Sep 30, 2022
5d3c6a3
Wrapping happens at 88 chars
MadLittleMods Sep 30, 2022
76435c7
Add actual homeserver tests for local/remote interesting to appservic…
MadLittleMods Sep 30, 2022
4451998
Clarify interested/control and lints
MadLittleMods Sep 30, 2022
1218f03
Revert mock
MadLittleMods Sep 30, 2022
7bd3803
Add test descriptions
MadLittleMods Sep 30, 2022
33f718c
Revert "Clarify interested/control and lints"
MadLittleMods Sep 30, 2022
cf8299b
Revert "Add test descriptions"
MadLittleMods Sep 30, 2022
3de90e6
Revert "Revert mock"
MadLittleMods Sep 30, 2022
ab33cd6
Revert "Add actual homeserver tests for local/remote interesting to a…
MadLittleMods Sep 30, 2022
3223512
Move tests over from #14000
MadLittleMods Oct 4, 2022
d913ceb
Merge branch 'develop' into madlittlemods/13942-appservice-get_users_…
MadLittleMods Oct 4, 2022
4f29e75
Merge branch 'develop' into madlittlemods/13942-appservice-get_users_…
MadLittleMods Oct 24, 2022
2665aa0
Update changelog
MadLittleMods Oct 24, 2022
39e2ead
Add specific test to make sure local interesting user events are pick…
MadLittleMods Oct 25, 2022
33a5b70
Update upgrade notes
MadLittleMods Oct 25, 2022
92400ff
move comma
anoadragon453 Oct 27, 2022
426ef5c
Merge branch 'develop' of github.com:matrix-org/synapse into madlittl…
anoadragon453 Oct 27, 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/13958.bugfix
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.
Copy link
Contributor Author

@MadLittleMods MadLittleMods Sep 30, 2022

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 the sync and presence code for someone who has more domain knowledge on what those things should exactly do.

For example #13967 (feel free to pick it up)

15 changes: 13 additions & 2 deletions synapse/appservice/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

@MadLittleMods MadLittleMods Sep 29, 2022

Choose a reason for hiding this comment

The 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(
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the behavior of is_interested_in_room slightly. Previously, we could match remote users when the appservice's user regex didn't include a hostname (all of our test cases and examples in the documentation don't). Was that just a bug?

Copy link
Member

Choose a reason for hiding this comment

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

Was that just a bug?

I don't see how an appservice could control remote users, so I think so...but might be worth asking someone with more info?

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Sep 30, 2022

Choose a reason for hiding this comment

The 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 get_users_in_room so they happily passed when I updated it to get_local_users_in_room as well 🤦‍♂️. I've added some actual homeserver integration tests and verified that they fail if we use get_local_users_in_room here ✅

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

though the historical context (which may be lost to time) is still very prevelant here.

Is there something specific to point to here? Or just from your memory?

Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,

That being said, the changes here are valid - simply only returning events targeted at local users covers the same use cases.

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 rooms namespace regex so this PR should be ok to merge.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down
16 changes: 14 additions & 2 deletions synapse/storage/databases/main/appservice.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,22 @@ async def get_app_service_users_in_room(
app_service: "ApplicationService",
cache_context: _CacheContext,
) -> List[str]:
users_in_room = await self.get_users_in_room(
"""
Get all users in a room that the appservice controls.

Args:
room_id: The room to check in.
app_service: The application service to check interest/control against

Returns:
List of user IDs that the appservice controls.
"""
# 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.
local_users_in_room = await self.get_local_users_in_room(
room_id, on_invalidate=cache_context.invalidate
)
return list(filter(app_service.is_interested_in_user, users_in_room))
return list(filter(app_service.is_interested_in_user, local_users_in_room))


class ApplicationServiceStore(ApplicationServiceWorkerStore):
Expand Down
19 changes: 12 additions & 7 deletions synapse/storage/databases/main/roommember.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,13 +152,18 @@ async def get_users_in_room(self, room_id: str) -> List[str]:
`get_current_hosts_in_room()` and so we can re-use the cache but it's
not horrible to have here either.

Uses `m.room.member`s in the room state at the current forward extremities to
determine which users are in the room.

Will return inaccurate results for rooms with partial state, since the state for
the forward extremities of those rooms will exclude most members. We may also
calculate room state incorrectly for such rooms and believe that a member is or
is not in the room when the opposite is true.
Uses `m.room.member`s in the room state at the current forward
extremities to determine which users are in the room.

Will return inaccurate results for rooms with partial state, since the
state for the forward extremities of those rooms will exclude most
members. We may also calculate room state incorrectly for such rooms and
believe that a member is or is not in the room when the opposite is
true.

Note: If you only care about users in the room local to the homeserver,
use `get_local_users_in_room(...)` instead which will be more
performant.
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
"""
return await self.db_pool.runInteraction(
"get_users_in_room", self.get_users_in_room_txn, room_id
Expand Down