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

Mark remote device list updates as already handled #12557

Merged
merged 3 commits into from
Apr 26, 2022
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/12557.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Reduce unnecessary work when handling remote device list updates.
5 changes: 3 additions & 2 deletions synapse/handlers/device.py
Original file line number Diff line number Diff line change
Expand Up @@ -505,8 +505,9 @@ async def notify_device_update(
"device_list_key", position, users={user_id}, rooms=room_ids
)

# We may need to do some processing asynchronously.
self._handle_new_device_update_async()
# We may need to do some processing asynchronously for local user IDs.
if self.hs.is_mine_id(user_id):
self._handle_new_device_update_async()
Comment on lines -508 to +510
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't fully understand the machinery here, but the docstring of _handle_new_device_update_async says:

Called when we have a new local device list update that we need to send out over federation.

So that seems reasonable.


async def notify_user_signature_update(
self, from_user_id: str, user_ids: List[str]
Expand Down
3 changes: 2 additions & 1 deletion synapse/storage/databases/main/devices.py
Original file line number Diff line number Diff line change
Expand Up @@ -1748,7 +1748,8 @@ def _add_device_outbound_room_poke_txn(
device_id,
room_id,
stream_id,
False,
# We only need to calculate outbound pokes for local users
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we never need to know (indeed, cannot know) the full set of hosts that a remote user shares via their rooms?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's more that we would never send out device list updates for remote users, so there is no need to calculate where to send the update to

not self.hs.is_mine_id(user_id),
encoded_context,
)
for room_id in room_ids
Expand Down
2 changes: 1 addition & 1 deletion tests/storage/test_devices.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def add_device_change(self, user_id, device_ids, host):
for device_id in device_ids:
stream_id = self.get_success(
self.store.add_device_change_to_streams(
"user_id", [device_id], ["!some:room"]
user_id, [device_id], ["!some:room"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this just plain broken before? (Why didn't we notice?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this unit test actually only cares about getting a stream_id out and inserting into add_device_list_outbound_pokes. TBH I'm not entirely convinced these unit tests are testing anything particular helpful, beyond basic "do my queries pull out what I just put into the DB?"

)
)

Expand Down