From ad8d7caecbb0cd75f5d7370b9f5c4645e87d621d Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 5 Jun 2023 15:09:10 +0200 Subject: [PATCH] feat(ui): Handle `RoomList`'s `State::Terminated` properly. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch handles errors from the Sliding Sync loop properly. When an error is received, the `RoomList` state machine enters the `Terminated` state. Immediately after that, the sync stream is stopped. When the sync stream is restarted, the next state will be calculated. When the current state is `Terminated`, the next state is the state that led to the `Terminated` state. To avoid having a “first” huge sync (imagine a room list of 1000 rooms, you don't want to “resume” from an error with a first sync for 1000 rooms), lists are partially “reset” depending of the state where the machine is in. An important test has been added to test all possibles cases with errors and list' states. --- crates/matrix-sdk-ui/src/room_list/mod.rs | 111 ++++-- .../tests/integration/room_list.rs | 337 ++++++++++++++++-- 2 files changed, 404 insertions(+), 44 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list/mod.rs b/crates/matrix-sdk-ui/src/room_list/mod.rs index e1305dbee..06c9a3f9c 100644 --- a/crates/matrix-sdk-ui/src/room_list/mod.rs +++ b/crates/matrix-sdk-ui/src/room_list/mod.rs @@ -152,12 +152,17 @@ impl RoomList { } Some(Err(error)) => { - // TODO: what to do when an error is raised? + let next_state = State::Terminated { from: Box::new(self.state.get()) }; + + Observable::set(&self.state, next_state); + yield Err(Error::SlidingSync(error)); + + break; } None => { - let next_state = State::Terminated; + let next_state = State::Terminated { from: Box::new(self.state.get()) }; Observable::set(&self.state, next_state); @@ -210,20 +215,26 @@ impl RoomList { match input { Viewport(ranges) => { - self.sliding_sync - .on_list(VISIBLE_ROOMS_LIST_NAME, |list| { - ready(list.set_sync_mode( - SlidingSyncMode::new_selective().add_ranges(ranges.clone()), - )) - }) - .await - .ok_or_else(|| Error::InputHasNotBeenApplied(Viewport(ranges)))?; + self.update_viewport(ranges).await?; } } Ok(()) } + async fn update_viewport(&self, ranges: Ranges) -> Result<(), Error> { + self.sliding_sync + .on_list(VISIBLE_ROOMS_LIST_NAME, |list| { + ready( + list.set_sync_mode(SlidingSyncMode::new_selective().add_ranges(ranges.clone())), + ) + }) + .await + .ok_or_else(|| Error::InputHasNotBeenApplied(Input::Viewport(ranges)))?; + + Ok(()) + } + /// Get a [`Room`] if it exists. pub async fn room(&self, room_id: &RoomId) -> Result { match self.sliding_sync.get_room(room_id).await { @@ -332,7 +343,7 @@ pub enum Error { } /// The state of the [`RoomList`]' state machine. -#[derive(Copy, Clone, Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub enum State { /// That's the first initial state. Init, @@ -349,7 +360,7 @@ pub enum State { /// At this state, the sync has been stopped (because it was requested, or /// because it has errored too many times previously). - Terminated, + Terminated { from: Box }, } impl State { @@ -363,7 +374,27 @@ impl State { FirstRooms => (AllRooms, Actions::first_rooms_are_loaded()), AllRooms => (Enjoy, Actions::none()), Enjoy => (Enjoy, Actions::none()), - Terminated => (Terminated, Actions::none()), + // If the state was `Terminated` but the next state is calculated again, it means the + // sync has been restarted. In this case, let's jump back on the previous state that led + // to the termination. No action is required in this scenario. + Terminated { from: previous_state } => { + match previous_state.as_ref() { + state @ Init | state @ FirstRooms => { + // Do nothing. + (state.to_owned(), Actions::none()) + } + + state @ AllRooms | state @ Enjoy => { + // Refresh the lists. + (state.to_owned(), Actions::refresh_lists()) + } + + Terminated { .. } => { + // Having `Terminated { from: Terminated { … } }` is not allowed. + unreachable!("It's impossible to reach `Terminated` from `Terminated`"); + } + } + } }; for action in actions.iter() { @@ -398,10 +429,10 @@ impl Action for AddVisibleRoomsList { } } -struct ChangeAllRoomsListToGrowingSyncMode; +struct SetAllRoomsListToGrowingSyncMode; #[async_trait] -impl Action for ChangeAllRoomsListToGrowingSyncMode { +impl Action for SetAllRoomsListToGrowingSyncMode { async fn run(&self, sliding_sync: &SlidingSync) -> Result<(), Error> { sliding_sync .on_list(ALL_ROOMS_LIST_NAME, |list| { @@ -453,7 +484,8 @@ macro_rules! actions { impl Actions { actions! { none => [], - first_rooms_are_loaded => [ChangeAllRoomsListToGrowingSyncMode, AddVisibleRoomsList], + first_rooms_are_loaded => [SetAllRoomsListToGrowingSyncMode, AddVisibleRoomsList], + refresh_lists => [SetAllRoomsListToGrowingSyncMode], } fn iter(&self) -> &[OneAction] { @@ -535,24 +567,59 @@ mod tests { let room_list = new_room_list().await?; let sliding_sync = room_list.sliding_sync(); + // First state. let state = State::Init; + // Hypothetical termination. + { + let state = + State::Terminated { from: Box::new(state.clone()) }.next(&sliding_sync).await?; + assert_eq!(state, State::Init); + } + + // Next state. let state = state.next(&sliding_sync).await?; assert_eq!(state, State::FirstRooms); + // Hypothetical termination. + { + let state = + State::Terminated { from: Box::new(state.clone()) }.next(&sliding_sync).await?; + assert_eq!(state, State::FirstRooms); + } + + // Next state. let state = state.next(&sliding_sync).await?; assert_eq!(state, State::AllRooms); + // Hypothetical termination. + { + let state = + State::Terminated { from: Box::new(state.clone()) }.next(&sliding_sync).await?; + assert_eq!(state, State::AllRooms); + } + + // Next state. let state = state.next(&sliding_sync).await?; assert_eq!(state, State::Enjoy); + // Hypothetical termination. + { + let state = + State::Terminated { from: Box::new(state.clone()) }.next(&sliding_sync).await?; + assert_eq!(state, State::Enjoy); + } + + // Next state. let state = state.next(&sliding_sync).await?; assert_eq!(state, State::Enjoy); - let state = State::Terminated; - - let state = state.next(&sliding_sync).await?; - assert_eq!(state, State::Terminated); + // Hypothetical termination. + { + let state = + State::Terminated { from: Box::new(state.clone()) }.next(&sliding_sync).await?; + assert_eq!(state, State::Enjoy); + } Ok(()) } @@ -583,7 +650,7 @@ mod tests { } #[async_test] - async fn test_action_change_all_rooms_list_to_growing_sync_mode() -> Result<(), Error> { + async fn test_action_set_all_rooms_list_to_growing_sync_mode() -> Result<(), Error> { let room_list = new_room_list().await?; let sliding_sync = room_list.sliding_sync(); @@ -602,7 +669,7 @@ mod tests { ); // Run the action! - ChangeAllRoomsListToGrowingSyncMode.run(sliding_sync).await.unwrap(); + SetAllRoomsListToGrowingSyncMode.run(sliding_sync).await.unwrap(); // List is still present, in Growing mode. assert_eq!( diff --git a/crates/matrix-sdk-ui/tests/integration/room_list.rs b/crates/matrix-sdk-ui/tests/integration/room_list.rs index 8e94d1c02..1da379f18 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list.rs @@ -40,22 +40,48 @@ impl Match for SlidingSyncMatcher { macro_rules! sync_then_assert_request_and_fake_response { ( [$server:ident, $room_list:ident, $room_list_sync_stream:ident] - $( states = $pre_state:ident -> $post_state:ident, )? + $( states = $pre_state:pat => $post_state:pat, )? assert request = { $( $request_json:tt )* }, - respond with = { $( $response_json:tt )* } + respond with = $( ( code $code:expr ) )? { $( $response_json:tt )* } + $(,)? + ) => { + sync_then_assert_request_and_fake_response! { + [$server, $room_list, $room_list_sync_stream] + sync matches Some(Ok(_)), + $( states = $pre_state => $post_state, )? + assert request = { $( $request_json )* }, + respond with = $( ( code $code ) )? { $( $response_json )* }, + } + }; + + ( + [$server:ident, $room_list:ident, $room_list_sync_stream:ident] + sync matches $sync_result:pat, + $( states = $pre_state:pat => $post_state:pat, )? + assert request = { $( $request_json:tt )* }, + respond with = $( ( code $code:expr ) )? { $( $response_json:tt )* } $(,)? ) => { { + let _code = 200; + $( let _code = $code; )? + let _mock_guard = Mock::given(SlidingSyncMatcher) - .respond_with(ResponseTemplate::new(200).set_body_json( + .respond_with(ResponseTemplate::new(_code).set_body_json( json!({ $( $response_json )* }) )) .mount_as_scoped(&$server) .await; - $( assert_eq!(State:: $pre_state, $room_list.state(), "pre state"); )? + $( + use State::*; - let next = $room_list_sync_stream.next().await.unwrap()?; + assert_matches!($room_list.state(), $pre_state, "pre state"); + )? + + let next = $room_list_sync_stream.next().await; + + assert_matches!(next, $sync_result, "sync's result"); for request in $server.received_requests().await.expect("Request recording has been disabled").iter().rev() { if SlidingSyncMatcher.matches(request) { @@ -74,7 +100,7 @@ macro_rules! sync_then_assert_request_and_fake_response { } } - $( assert_eq!(State:: $post_state, $room_list.state(), "post state"); )? + $( assert_matches!($room_list.state(), $post_state, "post state"); )? next } @@ -205,7 +231,7 @@ async fn test_sync_from_init_to_enjoy() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Init -> FirstRooms, + states = Init => FirstRooms, assert request = { "lists": { ALL_ROOMS: { @@ -247,7 +273,7 @@ async fn test_sync_from_init_to_enjoy() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = FirstRooms -> AllRooms, + states = FirstRooms => AllRooms, assert request = { "lists": { ALL_ROOMS: { @@ -288,7 +314,7 @@ async fn test_sync_from_init_to_enjoy() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = AllRooms -> Enjoy, + states = AllRooms => Enjoy, assert request = { "lists": { ALL_ROOMS: { @@ -321,7 +347,7 @@ async fn test_sync_from_init_to_enjoy() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Enjoy -> Enjoy, + states = Enjoy => Enjoy, assert request = { "lists": { ALL_ROOMS: { @@ -366,7 +392,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Init -> FirstRooms, + states = Init => FirstRooms, assert request = { "lists": { ALL_ROOMS: { @@ -394,7 +420,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = FirstRooms -> AllRooms, + states = FirstRooms => AllRooms, assert request = { "lists": { ALL_ROOMS: { @@ -402,8 +428,8 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { }, VISIBLE_ROOMS: { "ranges": [], - } - } + }, + }, }, respond with = { "pos": "1", @@ -429,7 +455,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = AllRooms -> Enjoy, + states = AllRooms => Enjoy, assert request = { "lists": { ALL_ROOMS: { @@ -460,6 +486,273 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { Ok(()) } +#[async_test] +async fn test_sync_resumes_from_terminated() -> Result<(), Error> { + let (server, room_list) = new_room_list().await?; + + let sync = room_list.sync(); + pin_mut!(sync); + + // Simulate an error from the `Init` state. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + sync matches Some(Err(_)), + states = Init => Terminated { .. }, + assert request = { + "lists": { + ALL_ROOMS: { + // The default range, in selective sync-mode. + "ranges": [[0, 19]], + }, + }, + }, + respond with = (code 400) { + "error": "foo", + "errcode": "M_UNKNOWN", + }, + }; + + // Ensure sync is terminated. + assert!(sync.next().await.is_none()); + + // Start a new sync. + let sync = room_list.sync(); + pin_mut!(sync); + + // Do a regular sync from the `Terminated` state. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Terminated { .. } => FirstRooms, + assert request = { + "lists": { + ALL_ROOMS: { + // Still the default range, in selective sync-mode. + "ranges": [[0, 19]], + }, + }, + }, + respond with = { + "pos": "1", + "lists": { + ALL_ROOMS: { + "count": 110, + }, + }, + "rooms": {}, + }, + }; + + // Simulate an error from the `FirstRooms` state. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + sync matches Some(Err(_)), + states = FirstRooms => Terminated { .. }, + assert request = { + "lists": { + ALL_ROOMS: { + // In `FirstRooms`, the sync-mode has changed to growing, with + // its initial range. + "ranges": [[0, 49]], + }, + VISIBLE_ROOMS: { + // Hello new list. + "ranges": [], + }, + }, + }, + respond with = (code 400) { + "error": "foo", + "errcode": "M_UNKNOWN", + }, + }; + + // Ensure sync is terminated. + assert!(sync.next().await.is_none()); + + // Start a new sync. + let sync = room_list.sync(); + pin_mut!(sync); + + // Update the viewport, just to be sure it's not reset later. + room_list.apply_input(Input::Viewport(vec![5..=10])).await?; + + // Do a regular sync from the `Terminated` state. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Terminated { .. } => AllRooms, + assert request = { + "lists": { + ALL_ROOMS: { + // In `AllRooms`, the sync-mode is still growing, but the range + // hasn't been modified due to previous error. + "ranges": [[0, 49]], + }, + VISIBLE_ROOMS: { + // We have set a viewport, which reflects here. + "ranges": [[5, 10]], + }, + }, + }, + respond with = { + "pos": "2", + "lists": { + ALL_ROOMS: { + "count": 110, + }, + }, + "rooms": {}, + }, + }; + + // Simulate an error from the `AllRooms` state. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + sync matches Some(Err(_)), + states = AllRooms => Terminated { .. }, + assert request = { + "lists": { + ALL_ROOMS: { + // In `AllRooms`, the sync-mode is still growing, and the range + // has made progress. + "ranges": [[0, 99]], + }, + VISIBLE_ROOMS: { + // Despites the error, the range is kept. + "ranges": [[5, 10]], + }, + }, + }, + respond with = (code 400) { + "error": "foo", + "errcode": "M_UNKNOWN", + }, + }; + + // Ensure sync is terminated. + assert!(sync.next().await.is_none()); + + // Start a new sync. + let sync = room_list.sync(); + pin_mut!(sync); + + // Do a regular sync from the `Terminated` state. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Terminated { .. } => Enjoy, + assert request = { + "lists": { + ALL_ROOMS: { + // Due to the error, the range is reset to its initial value. + "ranges": [[0, 49]], + }, + VISIBLE_ROOMS: { + // Despites the error, the range is kept. + "ranges": [[5, 10]], + }, + }, + }, + respond with = { + "pos": "3", + "lists": { + ALL_ROOMS: { + "count": 110, + }, + }, + "rooms": {}, + }, + }; + + // Do a regular sync from the `Enjoy` state to update the `ALL_ROOMS` list + // again. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Enjoy => Enjoy, + assert request = { + "lists": { + ALL_ROOMS: { + // No error. The range is making progress. + "ranges": [[0, 99]], + }, + VISIBLE_ROOMS: { + // No error. The range is still here. + "ranges": [[5, 10]], + }, + }, + }, + respond with = { + "pos": "4", + "lists": { + ALL_ROOMS: { + "count": 110, + }, + }, + "rooms": {}, + }, + }; + + // Simulate an error from the `Enjoy` state. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + sync matches Some(Err(_)), + states = Enjoy => Terminated { .. }, + assert request = { + "lists": { + ALL_ROOMS: { + // Range is making progress and is even reaching the maximum + // number of rooms. + "ranges": [[0, 109]], + }, + VISIBLE_ROOMS: { + // The range is still here. + "ranges": [[5, 10]], + }, + }, + }, + respond with = (code 400) { + "error": "foo", + "errcode": "M_UNKNOWN", + }, + }; + + // Ensure sync is terminated. + assert!(sync.next().await.is_none()); + + // Start a new sync. + let sync = room_list.sync(); + pin_mut!(sync); + + // Do a regular sync from the `Terminated` state. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Terminated { .. } => Enjoy, + assert request = { + "lists": { + ALL_ROOMS: { + // An error was received at the previous sync iteration. + // The list is still in growing sync-mode, but its range has + // been reset. + "ranges": [[0, 49]], + }, + VISIBLE_ROOMS: { + // The range is still here. + "ranges": [[5, 10]], + }, + }, + }, + respond with = { + "pos": "5", + "lists": { + ALL_ROOMS: { + "count": 110, + }, + }, + "rooms": {}, + }, + }; + + Ok(()) +} + #[async_test] async fn test_entries_stream() -> Result<(), Error> { let (server, room_list) = new_room_list().await?; @@ -472,7 +765,7 @@ async fn test_entries_stream() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Init -> FirstRooms, + states = Init => FirstRooms, assert request = { "lists": { ALL_ROOMS: { @@ -530,7 +823,7 @@ async fn test_entries_stream() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = FirstRooms -> AllRooms, + states = FirstRooms => AllRooms, assert request = { "lists": { ALL_ROOMS: { @@ -604,7 +897,7 @@ async fn test_entries_stream_with_updated_filter() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Init -> FirstRooms, + states = Init => FirstRooms, assert request = { "lists": { ALL_ROOMS: { @@ -658,7 +951,7 @@ async fn test_entries_stream_with_updated_filter() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = FirstRooms -> AllRooms, + states = FirstRooms => AllRooms, assert request = { "lists": { ALL_ROOMS: { @@ -1022,7 +1315,7 @@ async fn test_input_viewport() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Init -> FirstRooms, + states = Init => FirstRooms, assert request = { "lists": { ALL_ROOMS: { @@ -1039,7 +1332,7 @@ async fn test_input_viewport() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = FirstRooms -> AllRooms, + states = FirstRooms => AllRooms, assert request = { "lists": { ALL_ROOMS: { @@ -1062,7 +1355,7 @@ async fn test_input_viewport() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = AllRooms -> Enjoy, + states = AllRooms => Enjoy, assert request = { "lists": { ALL_ROOMS: {