From 5e662e855db925b380517bd94ef2b3622804a421 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 2 Sep 2024 10:16:11 +0200 Subject: [PATCH] fix(sdk): Don't subscribe to already subscribed rooms. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When subscribing to an already subscribed room, the subscription state was reset, the subscription was resent by cancelling the in-flight request. All this is useless and can create a feeling of lag. This patch checks if a room is subscribed first. If it is, nothing happens. If it is not, the room subscription is created, and the in-flight request is cancelled. Tests are updated to reflect this new change. Note: room subscription settings are not taken into account in this “presence” pattern. It means that if we subscribe to an already subscribed room but with different settings, nothing will happen. The server will ignore the new settings anywhere for the moment. --- .../tests/integration/room_list_service.rs | 40 ++++++++++++ crates/matrix-sdk/src/sliding_sync/mod.rs | 65 ++++++++++++++++--- 2 files changed, 95 insertions(+), 10 deletions(-) diff --git a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs index 89469e6add0..ce543093c20 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs @@ -2211,6 +2211,46 @@ async fn test_room_subscription() -> Result<(), Error> { }, }; + // Subscribe to an already subscribed room. Nothing happens. + + room_list.subscribe_to_rooms( + &[room_id_1], + Some(assign!(RoomSubscription::default(), { + required_state: vec![ + (StateEventType::RoomName, "".to_owned()), + (StateEventType::RoomTopic, "".to_owned()), + (StateEventType::RoomAvatar, "".to_owned()), + (StateEventType::RoomCanonicalAlias, "".to_owned()), + ], + timeline_limit: Some(uint!(30)), + })), + ); + + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + // strict comparison (with `=`) because we want to ensure + // the absence of `room_subscriptions`. + assert request = { + "conn_id": "room-list", + "lists": { + ALL_ROOMS: { + "ranges": [[0, 2]], + }, + }, + // NO `room_subscriptions`! + "extensions": { + "account_data": { "enabled": true }, + "receipts": { "enabled": true, "rooms": [ "*" ] }, + "typing": { "enabled": true }, + }, + }, + respond with = { + "pos": "3", + "lists": {}, + "rooms": {}, + }, + }; + Ok(()) } diff --git a/crates/matrix-sdk/src/sliding_sync/mod.rs b/crates/matrix-sdk/src/sliding_sync/mod.rs index fc083cd0eb8..82488167392 100644 --- a/crates/matrix-sdk/src/sliding_sync/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/mod.rs @@ -25,7 +25,7 @@ mod sticky_parameters; mod utils; use std::{ - collections::{BTreeMap, HashSet}, + collections::{btree_map::Entry, BTreeMap, HashSet}, fmt::Debug, future::Future, sync::{Arc, RwLock as StdRwLock}, @@ -145,6 +145,8 @@ impl SlidingSync { /// /// If the associated `Room`s exist, it will be marked as /// members are missing, so that it ensures to re-fetch all members. + /// + /// A subscription to an already subscribed room is ignored. pub fn subscribe_to_rooms( &self, room_ids: &[&RoomId], @@ -154,20 +156,31 @@ impl SlidingSync { let mut sticky = self.inner.sticky.write().unwrap(); let room_subscriptions = &mut sticky.data_mut().room_subscriptions; + let mut skip_sync_loop = false; + for room_id in room_ids { - if let Some(room) = self.inner.client.get_room(room_id) { - room.mark_members_missing(); + // If the room subscription already exists, let's not + // override it with a new one. First, it would reset its + // state (`RoomSubscriptionState`), and second it would try to + // re-subscribe with the next request. We don't want that. A room + // subscription should happen once, and next subscriptions should + // be ignored. + if let Entry::Vacant(entry) = room_subscriptions.entry((*room_id).to_owned()) { + if let Some(room) = self.inner.client.get_room(room_id) { + room.mark_members_missing(); + } + + entry.insert((RoomSubscriptionState::default(), settings.clone())); + + skip_sync_loop = true; } + } - room_subscriptions.insert( - (*room_id).to_owned(), - (RoomSubscriptionState::default(), settings.clone()), + if skip_sync_loop { + self.inner.internal_channel_send_if_possible( + SlidingSyncInternalMessage::SyncLoopSkipOverCurrentIteration, ); } - - self.inner.internal_channel_send_if_possible( - SlidingSyncInternalMessage::SyncLoopSkipOverCurrentIteration, - ); } /// Lookup a specific room @@ -1197,6 +1210,38 @@ mod tests { assert!(!room_subscriptions.contains_key(room_id_2)); } + // Subscribing to the same room doesn't reset the member sync state. + + { + struct MemberMatcher(OwnedRoomId); + + impl Match for MemberMatcher { + fn matches(&self, request: &Request) -> bool { + request.url.path() + == format!("/_matrix/client/r0/rooms/{room_id}/members", room_id = self.0) + && request.method == Method::GET + } + } + + let _mock_guard = Mock::given(MemberMatcher(room_id_0.to_owned())) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "chunk": [], + }))) + .mount_as_scoped(&server) + .await; + + assert_matches!(room0.request_members().await, Ok(())); + } + + // Members are synced, good, good. + assert!(room0.are_members_synced()); + + sliding_sync.subscribe_to_rooms(&[room_id_0], None); + + // Members are still synced: because we have already subscribed to the + // room, the members aren't marked as unsynced. + assert!(room0.are_members_synced()); + Ok(()) }