From c07b060080fbb43a3d5f6b13a103e4e8be673fd2 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 6 Apr 2023 10:49:15 +0200 Subject: [PATCH 1/7] feat(sdk): Create vectors with correct capacity to avoid reallocations. `updated_rooms` and `updated_lists` can be created with an initial capacity. Doing so will avoid reallocations if the vector is too small. --- crates/matrix-sdk/src/sliding_sync/mod.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk/src/sliding_sync/mod.rs b/crates/matrix-sdk/src/sliding_sync/mod.rs index df2c83612..73fab5830 100644 --- a/crates/matrix-sdk/src/sliding_sync/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/mod.rs @@ -354,9 +354,11 @@ impl SlidingSync { } let update_summary = { - let mut updated_rooms = Vec::new(); let mut rooms_map = self.inner.rooms.write().unwrap(); + // Update the rooms. + let mut updated_rooms = Vec::with_capacity(sliding_sync_response.rooms.len()); + for (room_id, mut room_data) in sliding_sync_response.rooms.into_iter() { // `sync_response` contains the rooms with decrypted events if any, so look at // the timeline events here first if the room exists. @@ -390,7 +392,8 @@ impl SlidingSync { updated_rooms.push(room_id); } - let mut updated_lists = Vec::new(); + // Update the lists. + let mut updated_lists = Vec::with_capacity(sliding_sync_response.lists.len()); for (name, updates) in sliding_sync_response.lists { let Some(list) = lists.get_mut(&name) else { From a71970d3def871f41e385d65d13f11881dc744a4 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 6 Apr 2023 10:50:20 +0200 Subject: [PATCH 2/7] chore(sdk): Rephrase a panic message. --- crates/matrix-sdk/src/sliding_sync/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk/src/sliding_sync/mod.rs b/crates/matrix-sdk/src/sliding_sync/mod.rs index 73fab5830..944f12c72 100644 --- a/crates/matrix-sdk/src/sliding_sync/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/mod.rs @@ -403,7 +403,7 @@ impl SlidingSync { }; let maximum_number_of_rooms: u32 = - updates.count.try_into().expect("the list total count convertible into u32"); + updates.count.try_into().expect("failed to convert `count` to `u32`"); if list.handle_response(maximum_number_of_rooms, &updates.ops)? { updated_lists.push(name.clone()); From d68a22c3786b069a496e117e1b7c6dd27bc1e2bc Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 6 Apr 2023 10:51:01 +0200 Subject: [PATCH 3/7] fix(sdk): `SlidingSyncList.room_list` emits updates for non-moving room. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `SlidingSyncList.room_list` field is used to store the list of rooms for a particular Sliding Sync list. It's used by the user to receive “diff”s when a room sees its position modified. For example when a new room receives a message, it “climbs back up” the entire room list to be at the top place. Another example is when a new room is created, some rooms will move around to give the new room a space. All those moves will create “diff”. However, the user also expects to receive a “diff” when a room has received some updates, even if it's position doesn't change. For example, when a room is already at the top of the list but receives a new message: It has received an update, but its position stays the same. This specific latter feature was implemented before, but it has been removed by accident in https://github.com/matrix-org/matrix-rust-sdk/ pull/1699 (more specifically in https://github.com/matrix-org/matrix- rust-sdk/pull/1699/commits/861a05be69a566d9a4ad125dc6ecb418d2b3210f). At that time, it was not clear why the code was filtering for specific filled room entires, to set the filled room entries, at the same position. Zero comment, zero test. I didn't consider this as a feature but as a bug. So this patch re-introduces this feature. Hopefully in a more optimal way. If a room has already triggered a first “diff” because of a position change, it won't trigger a second “diff” because it has received an update (which was the case before). The `SlidingSyncList::handle_response` method has also been renamed `update`, as it does update, whenever it comes from. The code has been commented and documented to explain this feature. Existing tests have been updated, especially for `apply_sync_operations` which now ensure the `rooms_that_have_received_an_update` collections is updated accordingly. Another test has been updated specifically to test the “diff”s received by `room_list`. --- .../matrix-sdk/src/sliding_sync/list/mod.rs | 148 +++++++++++++++--- crates/matrix-sdk/src/sliding_sync/mod.rs | 2 +- 2 files changed, 129 insertions(+), 21 deletions(-) diff --git a/crates/matrix-sdk/src/sliding_sync/list/mod.rs b/crates/matrix-sdk/src/sliding_sync/list/mod.rs index 57dfe6cc9..c8ecd2104 100644 --- a/crates/matrix-sdk/src/sliding_sync/list/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/list/mod.rs @@ -5,6 +5,7 @@ mod room_list_entry; use std::{ cmp::min, + collections::HashSet, fmt::Debug, iter, ops::Not, @@ -204,17 +205,34 @@ impl SlidingSyncList { self.inner.next_request() } - // Handle the response from the server. - #[instrument(skip(self, sync_operations), fields(name = self.name(), sync_operations_count = sync_operations.len()))] - pub(super) fn handle_response( + /// Update the list based on the response from the server. + /// + /// The `maximum_number_of_rooms` is the `lists.$this_list.count` value, + /// i.e. maximum number of available rooms as defined by the server. The + /// `list_sync_operations` is the `list.$this_list.ops` value + /// received from the server for this specific list. It consists of + /// operations to “move” rooms' positions. Finally, + /// the `rooms_that_have_received_an_update` is the `rooms` value received + /// from the server, which represents aggregated rooms that have received an + /// update. Maybe their position has changed, maybe they have received a new + /// event in their timeline. We need this information to update the + /// `room_list` even if the position of the room hasn't be modified: it + /// helps the user to know that a room has received an update. + #[instrument(skip(self, list_sync_operations), fields(name = self.name(), list_sync_operations_count = list_sync_operations.len()))] + pub(super) fn update( &mut self, maximum_number_of_rooms: u32, - sync_operations: &[v4::SyncOp], + list_sync_operations: &[v4::SyncOp], + rooms_that_have_received_an_update: &[OwnedRoomId], ) -> Result { - let result = self.inner.update_room_list(maximum_number_of_rooms, sync_operations)?; + let new_changes = self.inner.update_room_list( + maximum_number_of_rooms, + list_sync_operations, + rooms_that_have_received_an_update, + )?; self.inner.update_request_generator_state(maximum_number_of_rooms)?; - Ok(result) + Ok(new_changes) } // Reset `Self`. @@ -375,12 +393,25 @@ impl SlidingSyncListInner { }) } - // Update the [`Self::room_list`]. It also updates - // `[Self::maximum_number_of_rooms]`. + /// Update the [`Self::room_list`]. It also updates + /// `[Self::maximum_number_of_rooms]`. + /// + /// The `maximum_number_of_rooms` is the `lists.$this_list.count` value, + /// i.e. maximum number of available rooms as defined by the server. The + /// `list_sync_operations` is the `list.$this_list.ops` value + /// received from the server for this specific list. It consists of + /// operations to “move” rooms' positions. Finally, + /// the `rooms_that_have_received_an_update` is the `rooms` value received + /// from the server, which represents aggregated rooms that have received an + /// update. Maybe their position has changed, maybe they have received a new + /// event in their timeline. We need this information to update the + /// `room_list` even if the position of the room hasn't be modified: it + /// helps the user to know that a room has received an update. fn update_room_list( &self, maximum_number_of_rooms: u32, - sync_operations: &[v4::SyncOp], + list_sync_operations: &[v4::SyncOp], + rooms_that_have_received_an_update: &[OwnedRoomId], ) -> Result { let mut new_changes = false; @@ -399,8 +430,6 @@ impl SlidingSyncListInner { } // Update the `maximum_number_of_rooms` if it has changed. - // Again, it should happened only once at the beginning, as the value of - // `maximum_number_of_rooms` should not change over time. { let mut maximum_number_of_rooms_lock = self.maximum_number_of_rooms.write().unwrap(); @@ -411,13 +440,61 @@ impl SlidingSyncListInner { } } - // Run the sync operations. - if !sync_operations.is_empty() { + { let mut room_list = self.room_list.write().unwrap(); - apply_sync_operations(sync_operations, &mut room_list)?; + // Run the sync operations. + let mut rooms_that_have_received_an_update = + HashSet::from_iter(rooms_that_have_received_an_update.into_iter().cloned()); - new_changes = true; + if !list_sync_operations.is_empty() { + apply_sync_operations( + list_sync_operations, + &mut room_list, + &mut rooms_that_have_received_an_update, + )?; + + new_changes = true; + } + + // Finally, some rooms have received an update, but their position + // didn't change in the `room_list`, so no “diff” has + // been triggered. The `room_list` value is used by the + // user to know if a room has received an update: be either a + // position modification or an update in general (like a new room + // message). Let's trigger those. + let ranges = self.ranges.read().unwrap(); + + for (start, end) in ranges.iter().map(|(start, end)| { + (usize::try_from(*start).unwrap(), usize::try_from(*end).unwrap()) + }) { + let mut rooms_to_update = + Vec::with_capacity(rooms_that_have_received_an_update.len()); + + for (position, room_list_entry) in + room_list.iter().enumerate().skip(start).take(end.saturating_add(1)) + { + // Invalidated rooms must be considered as empty rooms, so let's just filter by + // filled rooms. + if let RoomListEntry::Filled(room_id) = room_list_entry { + // If room has received an update but that has not been handled by a + // sync operation. + if rooms_that_have_received_an_update.contains(room_id) { + rooms_to_update.push((position, room_list_entry.clone())); + } + } + } + + if !rooms_to_update.is_empty() { + for (position, room_list_entry) in rooms_to_update { + // Setting to `room_list`'s item to the same value, just + // to generate an “diff update”. + room_list.set(position, room_list_entry); + } + + new_changes = true; + } + } } Ok(new_changes) @@ -515,6 +592,7 @@ impl SlidingSyncListInner { fn apply_sync_operations( operations: &[v4::SyncOp], room_list: &mut ObservableVector, + rooms_that_have_received_an_update: &mut HashSet, ) -> Result<(), Error> { for operation in operations { match &operation.op { @@ -605,6 +683,10 @@ fn apply_sync_operations( for (room_entry_index, room_id) in room_entry_range.zip(room_ids) { // Syncing means updating the room list to `Filled`. room_list.set(room_entry_index, RoomListEntry::Filled(room_id.clone())); + + // This `room_id` has been handled, let's remove it from the rooms to handle + // later. + rooms_that_have_received_an_update.remove(room_id); } } @@ -629,11 +711,17 @@ fn apply_sync_operations( // `DELETE` for an index that doesn't exist. It happens with the existing // Sliding Sync Proxy (at the time of writing). It may be a bug or something // else. Anyway, it's better to consider an out-of-bounds `DELETE` as a no-op. - return Ok(()); + continue; } // Removing the entry in the room list. - room_list.remove(index); + let room_entry = room_list.remove(index); + + // This `room_id` has been handled, let's remove it from the rooms to handle + // later. + if let Some(room_id) = room_entry.as_room_id() { + rooms_that_have_received_an_update.remove(room_id); + } } // Specification says: @@ -666,6 +754,10 @@ fn apply_sync_operations( ))); } + // This `room_id` is being handled, let's remove it from the rooms to handle + // later. + rooms_that_have_received_an_update.remove(&room_id); + // Inserting a `Filled` entry in the room list . room_list.insert(index, RoomListEntry::Filled(room_id)); } @@ -727,9 +819,25 @@ fn apply_sync_operations( // // If the previous room list entry is `Filled`, it becomes `Invalidated`. // Otherwise, for `Empty` or `Invalidated`, it stays as is. - if let Some(RoomListEntry::Filled(room_id)) = room_list.get(room_entry_index) { - room_list - .set(room_entry_index, RoomListEntry::Invalidated(room_id.clone())); + match room_list.get(room_entry_index) { + Some(RoomListEntry::Filled(room_id)) => { + // This `room_id` is being handled, let's remove it from the rooms to + // handle later. + rooms_that_have_received_an_update.remove(room_id); + + room_list.set( + room_entry_index, + RoomListEntry::Invalidated(room_id.to_owned()), + ); + } + + Some(RoomListEntry::Invalidated(room_id)) => { + // This `room_id` has been handled, let's remove it from the rooms to + // handle later. + rooms_that_have_received_an_update.remove(room_id); + } + + _ => {} } } } diff --git a/crates/matrix-sdk/src/sliding_sync/mod.rs b/crates/matrix-sdk/src/sliding_sync/mod.rs index 944f12c72..c1b11a99b 100644 --- a/crates/matrix-sdk/src/sliding_sync/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/mod.rs @@ -405,7 +405,7 @@ impl SlidingSync { let maximum_number_of_rooms: u32 = updates.count.try_into().expect("failed to convert `count` to `u32`"); - if list.handle_response(maximum_number_of_rooms, &updates.ops)? { + if list.update(maximum_number_of_rooms, &updates.ops, &updated_rooms)? { updated_lists.push(name.clone()); } } From 828e36e3afd1097d087485e5fb9c09ba2d304d2d Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 6 Apr 2023 11:06:24 +0200 Subject: [PATCH 4/7] test(sdk): Test `apply_sync_operations` removes rooms that have been updated. --- .../matrix-sdk/src/sliding_sync/list/mod.rs | 129 +++++++++++------- 1 file changed, 83 insertions(+), 46 deletions(-) diff --git a/crates/matrix-sdk/src/sliding_sync/list/mod.rs b/crates/matrix-sdk/src/sliding_sync/list/mod.rs index c8ecd2104..b1c887c86 100644 --- a/crates/matrix-sdk/src/sliding_sync/list/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/list/mod.rs @@ -1252,7 +1252,7 @@ mod tests { })) .unwrap(); - list.handle_response(6, &[sync0]).unwrap(); + list.update(6, &[sync0], &[]).unwrap(); assert_eq!(list.get_room_id(0), Some(room0.to_owned())); assert_eq!(list.get_room_id(1), Some(room1.to_owned())); @@ -1289,7 +1289,7 @@ mod tests { ); // Fake a response. - let _ = $list.handle_response($maximum_number_of_rooms, &[]); + let _ = $list.update($maximum_number_of_rooms, &[], &[]); assert_eq!( $list.inner.request_generator.read().unwrap().is_fully_loaded(), @@ -1629,7 +1629,7 @@ mod tests { })) .unwrap(); - let new_changes = list.handle_response(5, &[sync]).unwrap(); + let new_changes = list.update(5, &[sync], &[]).unwrap(); assert!(new_changes); @@ -1730,16 +1730,18 @@ mod tests { macro_rules! assert_sync_operations { ( room_list = [ $( $room_list_entries:tt )* ], - operations = [ + sync_operations = [ $( { $( $operation:tt )+ } ),* $(,)? ] + $( , rooms = [ $( $rooms:literal ),* ] )? $(,)? => result = $result:tt, room_list = [ $( $expected_room_list_entries:tt )* ] + $( , rooms = [ $( $expected_rooms:literal ),* ] )? $(,)? ) => { let mut room_list = ObservableVector::from(entries![ $( $room_list_entries )* ]); @@ -1751,10 +1753,33 @@ mod tests { ),* ]; - let result = apply_sync_operations(operations, &mut room_list); + let mut rooms_that_have_received_an_update = HashSet::::new(); + + $( + { + $( + rooms_that_have_received_an_update.insert(room_id!( $rooms ).to_owned()); + )* + } + )? + + let result = apply_sync_operations(operations, &mut room_list, &mut rooms_that_have_received_an_update); assert!(result.$result()); assert_eq!(*room_list, entries![ $( $expected_room_list_entries )* ]); + + $( + #[allow(unused_mut)] + let mut expected_rooms_that_have_received_an_update = HashSet::::new(); + + { + $( + expected_rooms_that_have_received_an_update.insert(room_id!( $expected_rooms ).to_owned()); + )* + } + + assert_eq!(rooms_that_have_received_an_update, expected_rooms_that_have_received_an_update); + )? }; } @@ -1762,23 +1787,25 @@ mod tests { fn test_sync_operations_sync() { // All room list is updated. assert_sync_operations! { - room_list = [E, E, E], - operations = [ + room_list = [E, E, E, F("!r3:x.y")], + sync_operations = [ { "op": SlidingOp::Sync, "range": [0, 2], "room_ids": ["!r0:x.y", "!r1:x.y", "!r2:x.y"], } - ] + ], + rooms = ["!r0:x.y", "!r1:x.y", "!r2:x.y", "!r3:x.y"], // `r3` has received an update, but its position didn't change => result = is_ok, - room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], + room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y"), F("!r3:x.y")], + rooms = ["!r3:x.y"], }; // Partial update. assert_sync_operations! { room_list = [E, E, E], - operations = [ + sync_operations = [ { "op": SlidingOp::Sync, "range": [0, 1], @@ -1791,7 +1818,7 @@ mod tests { }; assert_sync_operations! { room_list = [E, E, E], - operations = [ + sync_operations = [ { "op": SlidingOp::Sync, "range": [1, 2], @@ -1809,7 +1836,7 @@ mod tests { // See https://github.com/matrix-org/sliding-sync/issues/52. assert_sync_operations! { room_list = [E], - operations = [ + sync_operations = [ { "op": SlidingOp::Sync, "range": [0, 9], // <- it should be [0, 0] @@ -1824,7 +1851,7 @@ mod tests { // Missing `range`. assert_sync_operations! { room_list = [E, E, E], - operations = [ + sync_operations = [ { "op": SlidingOp::Sync, "room_ids": ["!r0:x.y", "!r1:x.y"], @@ -1838,7 +1865,7 @@ mod tests { // Invalid `range`. assert_sync_operations! { room_list = [E, E, E], - operations = [ + sync_operations = [ { "op": SlidingOp::Sync, "range": [2, 0], @@ -1853,7 +1880,7 @@ mod tests { // Missing `room_ids`. assert_sync_operations! { room_list = [E, E, E], - operations = [ + sync_operations = [ { "op": SlidingOp::Sync, "range": [0, 2], @@ -1867,7 +1894,7 @@ mod tests { // Out of bounds operation. assert_sync_operations! { room_list = [E, E, E], - operations = [ + sync_operations = [ { "op": SlidingOp::Sync, "range": [2, 3], @@ -1889,7 +1916,7 @@ mod tests { // missing. assert_sync_operations! { room_list = [E, E, E], - operations = [ + sync_operations = [ { "op": SlidingOp::Sync, "range": [0, 2], @@ -1911,7 +1938,7 @@ mod tests { // room IDs. assert_sync_operations! { room_list = [E, E, E], - operations = [ + sync_operations = [ { "op": SlidingOp::Sync, "range": [0, 1], @@ -1935,21 +1962,23 @@ mod tests { // Delete a room entry in the middle. assert_sync_operations! { room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], - operations = [ + sync_operations = [ { "op": SlidingOp::Delete, "index": 1, } - ] + ], + rooms = ["!r0:x.y", "!r1:x.y"], // `r0` has also received an update. => result = is_ok, room_list = [F("!r0:x.y"), F("!r2:x.y")], + rooms = ["!r0:x.y"], }; // Delete a room entry at the beginning. assert_sync_operations! { room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], - operations = [ + sync_operations = [ { "op": SlidingOp::Delete, "index": 0, @@ -1963,7 +1992,7 @@ mod tests { // Delete a room entry at the end. assert_sync_operations! { room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], - operations = [ + sync_operations = [ { "op": SlidingOp::Delete, "index": 2, @@ -1977,7 +2006,7 @@ mod tests { // Delete an out of bounds room entry. assert_sync_operations! { room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], - operations = [ + sync_operations = [ { "op": SlidingOp::Delete, "index": 3, @@ -1994,22 +2023,24 @@ mod tests { // Insert a room entry in the middle. assert_sync_operations! { room_list = [E, E, E], - operations = [ + sync_operations = [ { "op": SlidingOp::Insert, "index": 1, "room_id": "!r1:x.y", } - ] + ], + rooms = ["!r0:x.y", "!r1:x.y"], // `r0` has also received an update. => result = is_ok, room_list = [E, F("!r1:x.y"), E, E], + rooms = ["!r0:x.y"], }; // Insert a room entry at the beginning. assert_sync_operations! { room_list = [E, E, E], - operations = [ + sync_operations = [ { "op": SlidingOp::Insert, "index": 0, @@ -2024,7 +2055,7 @@ mod tests { // Insert a room entry at the end assert_sync_operations! { room_list = [E, E, E], - operations = [ + sync_operations = [ { "op": SlidingOp::Insert, "index": 3, @@ -2039,7 +2070,7 @@ mod tests { // Insert an out of bounds room entry. assert_sync_operations! { room_list = [E, F("!r1:x.y"), E], - operations = [ + sync_operations = [ { "op": SlidingOp::Insert, "index": 4, @@ -2056,50 +2087,56 @@ mod tests { fn test_sync_operations_invalidate() { // Invalidating an empty room. assert_sync_operations! { - room_list = [E], - operations = [ + room_list = [E, F("!r1:x.y")], + sync_operations = [ { "op": SlidingOp::Invalidate, "range": [0, 0], } - ] + ], + rooms = ["!r1:x.y"], // `r1` has also received an update. => result = is_ok, - room_list = [E], + room_list = [E, F("!r1:x.y")], + rooms = ["!r1:x.y"], }; // Invalidating a filled room. assert_sync_operations! { - room_list = [F("!r0:x.y")], - operations = [ + room_list = [F("!r0:x.y"), F("!r1:x.y")], + sync_operations = [ { "op": SlidingOp::Invalidate, "range": [0, 0], } - ] + ], + rooms = ["!r0:x.y", "!r1:x.y"], // `r1` has also received an update. => result = is_ok, - room_list = [I("!r0:x.y")], + room_list = [I("!r0:x.y"), F("!r1:x.y")], + rooms = ["!r1:x.y"], }; // Invalidating an invalidated room. assert_sync_operations! { - room_list = [I("!r0:x.y")], - operations = [ + room_list = [I("!r0:x.y"), F("!r1:x.y")], + sync_operations = [ { "op": SlidingOp::Invalidate, "range": [0, 0], } - ] + ], + rooms = ["!r0:x.y", "!r1:x.y"], // `r1` has also received an update. => result = is_ok, - room_list = [I("!r0:x.y")], + room_list = [I("!r0:x.y"), F("!r1:x.y")], + rooms = ["!r1:x.y"], }; // Partial update. assert_sync_operations! { room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], - operations = [ + sync_operations = [ { "op": SlidingOp::Invalidate, "range": [0, 1], @@ -2111,7 +2148,7 @@ mod tests { }; assert_sync_operations! { room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], - operations = [ + sync_operations = [ { "op": SlidingOp::Invalidate, "range": [1, 2], @@ -2125,7 +2162,7 @@ mod tests { // Full update. assert_sync_operations! { room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], - operations = [ + sync_operations = [ { "op": SlidingOp::Invalidate, "range": [0, 2], @@ -2142,7 +2179,7 @@ mod tests { // See https://github.com/matrix-org/sliding-sync/issues/52. assert_sync_operations! { room_list = [F("!r0:x.y")], - operations = [ + sync_operations = [ { "op": SlidingOp::Invalidate, "range": [0, 9], // <- it should be [0, 0] @@ -2156,7 +2193,7 @@ mod tests { // Missing `range`. assert_sync_operations! { room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], - operations = [ + sync_operations = [ { "op": SlidingOp::Invalidate, } @@ -2169,7 +2206,7 @@ mod tests { // Invalid `range`. assert_sync_operations! { room_list = [F("!r0:x.y"), F("!r1:x.y"), F("!r2:x.y")], - operations = [ + sync_operations = [ { "op": SlidingOp::Invalidate, "range": [12, 0], From eb0e97b902982472bf19e6323478ac26c5543e82 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 6 Apr 2023 11:06:52 +0200 Subject: [PATCH 5/7] =?UTF-8?q?test(sdk):=20Test=20that=20`SlidingSyncList?= =?UTF-8?q?.room=5Flist`=20receives=20=E2=80=9Cdiff=E2=80=9D.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch updates a test to ensure that `room_list` receives “diff” for rooms that are modified by a sync operations, but also by another updates (like a new event). --- .../matrix-sdk/src/sliding_sync/list/mod.rs | 98 +++++++++++++------ 1 file changed, 68 insertions(+), 30 deletions(-) diff --git a/crates/matrix-sdk/src/sliding_sync/list/mod.rs b/crates/matrix-sdk/src/sliding_sync/list/mod.rs index b1c887c86..fae03ef95 100644 --- a/crates/matrix-sdk/src/sliding_sync/list/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/list/mod.rs @@ -906,9 +906,11 @@ impl SlidingSyncMode { mod tests { use std::ops::Deref; + use futures::StreamExt; use imbl::vector; use ruma::{api::client::sync::sync_events::v4::SlidingOp, assign, room_id, uint}; use serde_json::json; + use tokio::{spawn, sync::mpsc::unbounded_channel}; use super::*; @@ -1598,8 +1600,8 @@ mod tests { }; } - #[test] - fn test_sliding_sync_inner_update_state_room_lists_and_maximum_number_of_rooms() { + #[tokio::test] + async fn test_sliding_sync_inner_update_state_room_list_and_maximum_number_of_rooms() { let mut list = SlidingSyncList::builder() .name("foo") .sync_mode(SlidingSyncMode::Selective) @@ -1652,41 +1654,77 @@ mod tests { ); } - // Update the range. - for _ in 0..=1 { - // Simulate a request. - let _ = list.next_request(); + let mut room_list_stream = list.room_list_stream(); + let (room_list_stream_sender, mut room_list_stream_receiver) = unbounded_channel(); - // A new response. - let sync: v4::SyncOp = serde_json::from_value(json!({ - "op": SlidingOp::Sync, - "range": [3, 4], - "room_ids": [room3, room4], - })) + spawn(async move { + while let Some(diff) = room_list_stream.next().await { + room_list_stream_sender.send(diff).unwrap(); + } + }); + + // Simulate a request. + let _ = list.next_request(); + + // A new response. + let sync: v4::SyncOp = serde_json::from_value(json!({ + "op": SlidingOp::Sync, + "range": [3, 4], + "room_ids": [room3, room4], + })) + .unwrap(); + + let new_changes = list + .update( + 5, + &[sync], + // Let's imagine `room2` has received an update, but its position doesn't + // change. + &[room3.to_owned(), room4.to_owned(), room2.to_owned()], + ) .unwrap(); - let new_changes = list.handle_response(5, &[sync]).unwrap(); + assert!(new_changes); - assert!(new_changes); + // The `maximum_number_of_rooms` has been updated as expected. + assert_eq!(**list.inner.maximum_number_of_rooms.read().unwrap(), Some(5)); - // The `maximum_number_of_rooms` has been updated as expected. - assert_eq!(**list.inner.maximum_number_of_rooms.read().unwrap(), Some(5)); + // The `room_list` has the correct size and contains expected room entries. + let room_list = list.inner.room_list.read().unwrap(); - // The `room_list` has the correct size and contains expected room entries. - let room_list = list.inner.room_list.read().unwrap(); + assert_eq!(room_list.len(), 5); + assert_eq!( + **room_list, + vector![ + RoomListEntry::Filled(room0.to_owned()), + RoomListEntry::Filled(room1.to_owned()), + RoomListEntry::Filled(room2.to_owned()), + RoomListEntry::Filled(room3.to_owned()), + RoomListEntry::Filled(room4.to_owned()), + ] + ); - assert_eq!(room_list.len(), 5); - assert_eq!( - **room_list, - vector![ - RoomListEntry::Filled(room0.to_owned()), - RoomListEntry::Filled(room1.to_owned()), - RoomListEntry::Filled(room2.to_owned()), - RoomListEntry::Filled(room3.to_owned()), - RoomListEntry::Filled(room4.to_owned()), - ] - ); - } + // Wait for “diff” to be generated. + // Why this? Because the following `assert_eq` are using `try_recv()` instead of + // recv().await`, so that a missing “diff” doesn't make the test to hang + // forever. + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + + // `room3` has been modified by a `SYNC` operation. + assert_eq!( + room_list_stream_receiver.try_recv(), + Ok(VectorDiff::Set { index: 3, value: RoomListEntry::Filled(room3.to_owned()) }) + ); + // `room4` has been modified by a `SYNC` operation. + assert_eq!( + room_list_stream_receiver.try_recv(), + Ok(VectorDiff::Set { index: 4, value: RoomListEntry::Filled(room4.to_owned()) }) + ); + // `room2` has been modified by another update (like a new event). + assert_eq!( + room_list_stream_receiver.try_recv(), + Ok(VectorDiff::Set { index: 2, value: RoomListEntry::Filled(room2.to_owned()) }) + ); } #[test] From fbe162a1bc6779ea3c281a6126eeed8ae004ebbe Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 6 Apr 2023 11:29:10 +0200 Subject: [PATCH 6/7] test(sdk): Fix an integration test in Sliding Sync. The test does the following: 1. Create 2 (identical) lists, 2. Do a sync. 3. Assert that the 1st and 2nd lists are receiving an update, 4. Add a 3rd (identical) list, 5. Do a new sync, 6. Assert that 3rd list is receiving an update. This last step is wrong. All lists should receive an update as they are identical. --- testing/sliding-sync-integration-test/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/sliding-sync-integration-test/src/lib.rs b/testing/sliding-sync-integration-test/src/lib.rs index 78628a45f..10ab983e1 100644 --- a/testing/sliding-sync-integration-test/src/lib.rs +++ b/testing/sliding-sync-integration-test/src/lib.rs @@ -360,7 +360,7 @@ async fn adding_list_later() -> anyhow::Result<()> { // we only heard about the ones we had asked for if !summary.lists.is_empty() { // only if we saw an update come through - assert_eq!(summary.lists, [list_name_3]); + assert_eq!(summary.lists, [list_name_1, list_name_2, list_name_3]); // we didn't update the other lists, so only no 2 should se an update saw_update = true; break; From 4ee49065364c55ab390aa79ec325719a7fc86e90 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 6 Apr 2023 11:36:37 +0200 Subject: [PATCH 7/7] chore(sdk): Make Clippy happy. --- crates/matrix-sdk/src/sliding_sync/list/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk/src/sliding_sync/list/mod.rs b/crates/matrix-sdk/src/sliding_sync/list/mod.rs index fae03ef95..3e2f283f5 100644 --- a/crates/matrix-sdk/src/sliding_sync/list/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/list/mod.rs @@ -445,7 +445,7 @@ impl SlidingSyncListInner { // Run the sync operations. let mut rooms_that_have_received_an_update = - HashSet::from_iter(rooms_that_have_received_an_update.into_iter().cloned()); + HashSet::from_iter(rooms_that_have_received_an_update.iter().cloned()); if !list_sync_operations.is_empty() { apply_sync_operations( @@ -1601,6 +1601,7 @@ mod tests { } #[tokio::test] + #[allow(clippy::await_holding_lock)] async fn test_sliding_sync_inner_update_state_room_list_and_maximum_number_of_rooms() { let mut list = SlidingSyncList::builder() .name("foo")