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

Only notify the target of a membership event #14971

Merged
merged 3 commits into from
Feb 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/14971.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improve performance of joining and leaving large rooms with many local users.
38 changes: 30 additions & 8 deletions synapse/push/bulk_push_rule_evaluator.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,34 @@ async def _get_rules_for_event(
Returns:
Mapping of user ID to their push rules.
"""
# We get the users who may need to be notified by first fetching the
# local users currently in the room, finding those that have push rules,
# and *then* checking which users are actually allowed to see the event.
#
# The alternative is to first fetch all users that were joined at the
# event, but that requires fetching the full state at the event, which
# may be expensive for large rooms with few local users.
# If this is a membership event, only calculate push rules for the target.
# While it's possible for users to configure push rules to respond to such an
# event, in practise nobody does this. At the cost of violating the spec a
# little, we can skip fetching a huge number of push rules in large rooms.
# This helps make joins and leaves faster.
if event.type == EventTypes.Member:
local_users = []
# We never notify a user about their own actions. This is enforced in
# `_action_for_event_by_user` in the loop over `rules_by_user`, but we
# do the same check here to avoid unnecessary DB queries.
if event.sender != event.state_key and self.hs.is_mine_id(event.state_key):
# Check the target is in the room, to avoid notifying them of
# e.g. a pre-emptive ban.
target_already_in_room = await self.store.check_local_user_in_room(
event.state_key, event.room_id
)
if target_already_in_room:
local_users = [event.state_key]
Comment on lines +158 to +159
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If not target_already_in_room then we don't give local_users a value 🤦.

We were surprised that the linting didn't spot this. I opened astral-sh/ruff#2493 to see what the ruff author(s?) think.

else:
# We get the users who may need to be notified by first fetching the
# local users currently in the room, finding those that have push rules,
# and *then* checking which users are actually allowed to see the event.
#
# The alternative is to first fetch all users that were joined at the
# event, but that requires fetching the full state at the event, which
# may be expensive for large rooms with few local users.

local_users = await self.store.get_local_users_in_room(event.room_id)
local_users = await self.store.get_local_users_in_room(event.room_id)

# Filter out appservice users.
local_users = [
Expand All @@ -164,6 +183,9 @@ async def _get_rules_for_event(
local_users = list(local_users)
local_users.append(invited)

if not local_users:
return {}

rules_by_user = await self.store.bulk_get_push_rules(local_users)

logger.debug("Users in room: %s", local_users)
Expand Down