feat(room): add `Room::remove_outdated_seen_knock_requests_ids` method #4422

jmartinesp · 2024-12-17T10:53:59Z

In a previous PR (#4338) we introduced the concept of a 'seen' knock request id, that is, a member event with knock membership that the current user has seen and decided to explicitly ignore so it can be filtered out in the future in the clients and won't clutter the UI. However, if left alone this list will only grow indefinitely, so we need a way to observe the changes in knock room members in the room and remove outdated seen request ids.

Take this PR as a follow-up of the one mentioned above.

A new matrix_sdk_base::room_member_updates_sender field was added too, which will broadcast when a room members update is received, both when loading the room members from the HS and when new member state events are received in a sync.

On fn Room::subscribe_to_knock_requests a new cleanup task is also spawned to listen to these broadcast and call the fn Room::remove_outdated_seen_knock_requests_ids, which will compare the existing seen knock request ids with the room members having a knock membership state in the room and discard the seen knock request ids that are no longer valid.

Public API changes documented in changelogs (optional)

Signed-off-by:

jmartinesp · 2024-12-17T10:54:58Z

crates/matrix-sdk-base/src/rooms/normal.rs

+pub enum RoomMembersUpdateKind {
+    /// The whole list room members was reloaded.
+    FullReload,
+    /// A few members were updated, their user ids are included.
+    Partial(BTreeSet<OwnedUserId>),


While this distinction is not used at the moment, I hope it'll make building a live room members observable in the future a bit easier.

codecov · 2024-12-17T11:08:58Z

Codecov Report

Attention: Patch coverage is 94.54545% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.44%. Comparing base (f18e0b1) to head (f53a16d).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-base/src/rooms/normal.rs	95.12%	2 Missing ⚠️
crates/matrix-sdk/src/room/mod.rs	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4422      +/-   ##
==========================================
+ Coverage   85.41%   85.44%   +0.02%     
==========================================
  Files         283      283              
  Lines       31490    31528      +38     
==========================================
+ Hits        26897    26938      +41     
+ Misses       4593     4590       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr · 2024-12-18T13:43:44Z

crates/matrix-sdk/src/event_handler/mod.rs

@@ -1167,4 +1167,79 @@ mod tests {

        Ok(())
    }
+
+    #[async_test]
+    #[allow(dependency_on_unit_never_type_fallback)]


Can we get rid of it, or comment why it's needed?

crates/matrix-sdk-base/src/client.rs

bnjbvr · 2024-12-18T13:45:35Z

crates/matrix-sdk-base/src/client.rs

+        if let Err(err) = room.room_member_updates_sender.send(RoomMembersUpdateKind::FullReload) {
+            warn!("Failed to broadcast full room members reload: {err}");
+        }


Ditto here.

bnjbvr · 2024-12-18T13:46:02Z

crates/matrix-sdk-base/src/rooms/normal.rs

@@ -262,6 +265,15 @@ fn heroes_filter<'a>(
    move |user_id| user_id != own_user_id && !member_hints.service_members.contains(user_id)
 }

+/// The kind of room member updates that just happened.
+#[derive(Debug, Clone)]
+pub enum RoomMembersUpdateKind {


Could this be called RoomMembersUpdate instead?

bnjbvr · 2024-12-18T13:46:54Z

crates/matrix-sdk-base/src/rooms/normal.rs

@@ -297,6 +309,7 @@ impl Room {
            ))),
            room_info_notable_update_sender,
            seen_knock_request_ids_map: SharedObservable::new_async(None),
+            room_member_updates_sender: broadcast::channel(10).0,


Can you use local variables instead? It's really not clear what the 0 is, so before assigning the sender here, you'd have a line above like let (sender, _receiver) = broadcast::channel(10);, and then use sender here.

crates/matrix-sdk/src/room/mod.rs

bnjbvr · 2024-12-18T13:51:20Z

crates/matrix-sdk-base/src/rooms/normal.rs

@@ -174,6 +174,9 @@ pub struct Room {
    /// user has marked as seen so they can be ignored.
    pub seen_knock_request_ids_map:
        SharedObservable<Option<BTreeMap<OwnedEventId, OwnedUserId>>, AsyncLock>,
+
+    /// A sender that will notify receivers when room member updates happen.
+    pub room_member_updates_sender: broadcast::Sender<RoomMembersUpdateKind>,


Since this is pub, it's part of the public API, and it must be tested that it works correctly when subscribing 😈

bnjbvr · 2024-12-18T13:52:29Z

crates/matrix-sdk-base/src/rooms/normal.rs

+        // We're not calling `get_seen_join_request_ids` here because we need to keep
+        // the Mutex's guard until we've updated the data
+        let mut current_seen_events = if current_seen_events_guard.is_none() {
+            self.load_cached_knock_request_ids().await?
+        } else {
+            current_seen_events_guard.clone().unwrap()
+        };


Think this is the third duplication of this piece of code, can we refactor it into a small helper?

bnjbvr · 2024-12-18T13:52:50Z

crates/matrix-sdk-base/src/rooms/normal.rs

+        // If there is nothing to compare the current seen knock ids to, return early
+        if not_knocked_user_ids.is_empty() {
+            return Ok(());
+        }


Can you put this code before we take the seen_knock_request_ids_map mutex?

bnjbvr · 2024-12-18T13:55:09Z

crates/matrix-sdk-base/src/rooms/normal.rs

+        let mut to_remove = Vec::new();
+        for (event_id, user_id) in current_seen_events.iter() {
+            if not_knocked_user_ids.contains(user_id) {
+                to_remove.push(event_id.to_owned());
+            }
+        }


I think the approach you've implemented works, but it does a lot of work when the room is large and when there's few members who knocked. Instead, could we iterate over all the user ids from the current_seen_events knock events, get their current membership from the store, and remove them if their own membership has changed?

Now that will mean that instead of loading N memberships (when N is the number of members in the room), we'll only load as many as the number of members who knocked.

jmartinesp · 2024-12-18T17:29:03Z

I added fixes to your comments and as you suggested I reorganised the commits (I hope they're easier to review now 🤞 ). I also added a couple of more isolated tests for Room::remove_outdated_seen_knock_requests_ids and extracted the test for Room::observe_events into a different PR.

bnjbvr

Thanks for the splitting of commits, it helps a lot with the reviewing 🙏

I added a suggestion for not having to reload all member events from the database, as I think it can work.

bnjbvr · 2024-12-19T09:49:50Z

crates/matrix-sdk-base/src/rooms/normal.rs

+        room.room_member_updates_sender
+            .send(RoomMembersUpdate::FullReload)
+            .expect("broadcasting a room members update failed");


I don't think we should send something manually; instead, we should test that the described behavior (will be pinged upon /members or incremental room member update) is correct. So we'd need two tests:

when I'm calling /members, then I get a FullReload

when I'm receiving new member info from sync, then I get Partial with the correct user id set

Does it make sense?

bnjbvr · 2024-12-19T09:51:18Z

crates/matrix-sdk-base/src/rooms/normal.rs

+        // We're not calling `get_seen_join_request_ids` here because we need to keep
+        // the Mutex's guard until we've updated the data


Think this comment can go now?

bnjbvr · 2024-12-19T09:54:38Z

crates/matrix-sdk-base/src/rooms/normal.rs

+        let raw_member_events: Vec<RawMemberEvent> =
+            self.store.get_state_events_for_keys_static(self.room_id(), &keys).await?;
+        let member_events = raw_member_events
+            .into_iter()
+            .map(|raw| raw.deserialize())
+            .collect::<Result<Vec<MemberEvent>, _>>()?;


It's a bit unfortunate that we have to load all the member events from the database systematically, only to filter them out later. The base crate has BaseRoom::get_member(user_id) and StateStore::get_member_event(user_id), so we can do better. In pseudo-code:

for each (event_id, user_id) in current_seen_events { let member = store.get_member(room_id, user_id); if member.state is not knocked { remove it from current_seen_events } }

And that way, we don't have to preload all the member events from the database. Does it make sense?

But wouldn't that mean we now need to do N queries instead of a single one? I had that code before (with store.get_member_event) and I thought it wasn't as performant as it could be:

With store.get_member_event you make several queries.

With room.get_member you make several queries and also load the presence and other data that's not necessary at this point.

The keys passed are the user ids of the seen knock requests, so I don't think we're doing any extra work vs loading the members one by one.

The keys passed are the user ids of the seen knock requests, so I don't think we're doing any extra work vs loading the members one by one.

Alright, I missed that, then we should be good to go 👍

jmartinesp · 2024-12-19T11:29:36Z

ec1c806 should take care of those issues. Sorry I force pushed, I realised I had messed up a doc comment just after uploading the new commit.

bnjbvr

Thanks, looks great!

This sender will notify receivers when new room members are received: this can happen either when reloading the full room member list from the HS or when new member events arrive during a sync. The sender will emit a `RoomMembersUpdate`, which can be either a full reload or a partial one, including the user ids of the members that changed.

…rent seen knock request ids while keeping them thread-safe with a lock around them

This will check the current seen knock request ids against the room members related to them and will remove those seen ids for members which are no longer in knock state or come from an outdated knock member event.

This cleanup task will run while the knock request subscription runs and will use the `Room::room_member_updates_sender` notification to call `Room::remove_outdated_seen_knock_requests_ids` and remove outdated seen knock request ids automatically.

jmartinesp requested a review from a team as a code owner December 17, 2024 10:53

jmartinesp requested review from andybalaam and removed request for a team December 17, 2024 10:54

jmartinesp commented Dec 17, 2024

View reviewed changes

jmartinesp requested review from bnjbvr and removed request for andybalaam December 17, 2024 10:57

bnjbvr requested changes Dec 18, 2024

View reviewed changes

jmartinesp force-pushed the feat/remove-seen-knock-request-ids branch 3 times, most recently from 06c4332 to 8f23be3 Compare December 18, 2024 17:26

jmartinesp force-pushed the feat/remove-seen-knock-request-ids branch from 8f23be3 to 9cf7e91 Compare December 18, 2024 17:29

bnjbvr self-requested a review December 19, 2024 09:45

bnjbvr requested changes Dec 19, 2024

View reviewed changes

jmartinesp requested a review from bnjbvr December 19, 2024 11:26

jmartinesp force-pushed the feat/remove-seen-knock-request-ids branch 2 times, most recently from a6b3976 to ec1c806 Compare December 19, 2024 11:27

bnjbvr approved these changes Dec 19, 2024

View reviewed changes

jmartinesp added 5 commits December 19, 2024 13:39

feat(room): add helper methods to BaseRoom to get and write the cur…

34f0200

…rent seen knock request ids while keeping them thread-safe with a lock around them

feat(room): add BaseRoom::remove_outdated_seen_knock_requests_ids fn

21c15a6

This will check the current seen knock request ids against the room members related to them and will remove those seen ids for members which are no longer in knock state or come from an outdated knock member event.

test(room): Improve Room::room_member_updates_sender tests

f53a16d

jmartinesp force-pushed the feat/remove-seen-knock-request-ids branch from ec1c806 to f53a16d Compare December 19, 2024 12:39

jmartinesp enabled auto-merge (rebase) December 19, 2024 12:40

jmartinesp merged commit 38cc9fb into main Dec 19, 2024
39 checks passed

jmartinesp deleted the feat/remove-seen-knock-request-ids branch December 19, 2024 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(room): add `Room::remove_outdated_seen_knock_requests_ids` method #4422

feat(room): add `Room::remove_outdated_seen_knock_requests_ids` method #4422

jmartinesp commented Dec 17, 2024 •

edited

Loading

jmartinesp Dec 17, 2024

codecov bot commented Dec 17, 2024 •

edited

Loading

bnjbvr Dec 18, 2024

bnjbvr Dec 18, 2024

bnjbvr Dec 18, 2024

bnjbvr Dec 18, 2024

bnjbvr Dec 18, 2024

bnjbvr Dec 18, 2024

bnjbvr Dec 18, 2024

bnjbvr Dec 18, 2024

jmartinesp commented Dec 18, 2024

bnjbvr left a comment

bnjbvr Dec 19, 2024

bnjbvr Dec 19, 2024

bnjbvr Dec 19, 2024

jmartinesp Dec 19, 2024 •

edited

Loading

bnjbvr Dec 19, 2024

jmartinesp commented Dec 19, 2024 •

edited

Loading

bnjbvr left a comment

		// We're not calling `get_seen_join_request_ids` here because we need to keep
		// the Mutex's guard until we've updated the data

feat(room): add Room::remove_outdated_seen_knock_requests_ids method #4422

feat(room): add Room::remove_outdated_seen_knock_requests_ids method #4422

Conversation

jmartinesp commented Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Dec 18, 2024

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Dec 19, 2024 • edited Loading

bnjbvr left a comment

Choose a reason for hiding this comment

feat(room): add `Room::remove_outdated_seen_knock_requests_ids` method #4422

feat(room): add `Room::remove_outdated_seen_knock_requests_ids` method #4422

jmartinesp commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

jmartinesp Dec 19, 2024 •

edited

Loading

jmartinesp commented Dec 19, 2024 •

edited

Loading