fix(sdk): Don't subscribe to already subscribed rooms.

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.

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(())
 }
 

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(())
     }