From 373709fb38c28aa182bd6d94c588bf9c89ad7e1d Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Mon, 16 Dec 2024 18:36:50 +0100 Subject: [PATCH] feat(event cache): don't replace a gap chunk by an empty items chunks --- .../matrix-sdk-common/src/linked_chunk/mod.rs | 139 +++++++++++++++--- .../matrix-sdk/src/event_cache/pagination.rs | 9 +- .../matrix-sdk/src/event_cache/room/events.rs | 60 ++++++-- .../tests/integration/event_cache.rs | 73 ++++++++- 4 files changed, 246 insertions(+), 35 deletions(-) diff --git a/crates/matrix-sdk-common/src/linked_chunk/mod.rs b/crates/matrix-sdk-common/src/linked_chunk/mod.rs index f42409d68f7..3294f58845f 100644 --- a/crates/matrix-sdk-common/src/linked_chunk/mod.rs +++ b/crates/matrix-sdk-common/src/linked_chunk/mod.rs @@ -634,6 +634,47 @@ impl LinkedChunk { Ok(()) } + /// Remove a gap with the given identifier. + /// + /// This returns the next insert position, viz. the start of the next + /// chunk, if any, or none if there was no next chunk. + pub fn remove_gap_at( + &mut self, + chunk_identifier: ChunkIdentifier, + ) -> Result, Error> { + let chunk = self + .links + .chunk_mut(chunk_identifier) + .ok_or(Error::InvalidChunkIdentifier { identifier: chunk_identifier })?; + + if chunk.is_items() { + return Err(Error::ChunkIsItems { identifier: chunk_identifier }); + }; + + let next = chunk.next; + + chunk.unlink(&mut self.updates); + + let chunk_ptr = chunk.as_ptr(); + + // If this ever changes, we may need to update self.links.first too. + debug_assert!(chunk.is_first_chunk().not(), "A gap cannot be the first chunk"); + + if chunk.is_last_chunk() { + self.links.last = chunk.previous; + } + + // SAFETY: `chunk` is unlinked and not borrowed anymore. `LinkedChunk` doesn't + // use it anymore, it's a leak. It is time to re-`Box` it and drop it. + let _chunk_boxed = unsafe { Box::from_raw(chunk_ptr.as_ptr()) }; + + // Return the first position of the next chunk, if any. + Ok(next.map(|next| { + let chunk = unsafe { next.as_ref() }; + chunk.first_position() + })) + } + /// Replace the gap identified by `chunk_identifier`, by items. /// /// Because the `chunk_identifier` can represent non-gap chunk, this method @@ -661,27 +702,25 @@ impl LinkedChunk { .chunk_mut(chunk_identifier) .ok_or(Error::InvalidChunkIdentifier { identifier: chunk_identifier })?; - debug_assert!(chunk.is_first_chunk().not(), "A gap cannot be the first chunk"); + if chunk.is_items() { + return Err(Error::ChunkIsItems { identifier: chunk_identifier }); + }; - let maybe_last_chunk_ptr = match &mut chunk.content { - ChunkContent::Gap(..) => { - let items = items.into_iter(); + debug_assert!(chunk.is_first_chunk().not(), "A gap cannot be the first chunk"); - let last_inserted_chunk = chunk - // Insert a new items chunk… - .insert_next( - Chunk::new_items_leaked(self.chunk_identifier_generator.next()), - &mut self.updates, - ) - // … and insert the items. - .push_items(items, &self.chunk_identifier_generator, &mut self.updates); + let maybe_last_chunk_ptr = { + let items = items.into_iter(); - last_inserted_chunk.is_last_chunk().then(|| last_inserted_chunk.as_ptr()) - } + let last_inserted_chunk = chunk + // Insert a new items chunk… + .insert_next( + Chunk::new_items_leaked(self.chunk_identifier_generator.next()), + &mut self.updates, + ) + // … and insert the items. + .push_items(items, &self.chunk_identifier_generator, &mut self.updates); - ChunkContent::Items(..) => { - return Err(Error::ChunkIsItems { identifier: chunk_identifier }) - } + last_inserted_chunk.is_last_chunk().then(|| last_inserted_chunk.as_ptr()) }; new_chunk_ptr = chunk @@ -2691,6 +2730,72 @@ mod tests { Ok(()) } + #[test] + fn test_remove_gap() -> Result<(), Error> { + use super::Update::*; + + let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); + + // Ignore initial update. + let _ = linked_chunk.updates().unwrap().take(); + + linked_chunk.push_items_back(['a', 'b']); + linked_chunk.push_gap_back(()); + linked_chunk.push_items_back(['l', 'm']); + linked_chunk.push_gap_back(()); + assert_items_eq!(linked_chunk, ['a', 'b'] [-] ['l', 'm'] [-]); + assert_eq!( + linked_chunk.updates().unwrap().take(), + &[ + PushItems { at: Position(ChunkIdentifier(0), 0), items: vec!['a', 'b'] }, + NewGapChunk { + previous: Some(ChunkIdentifier(0)), + new: ChunkIdentifier(1), + next: None, + gap: (), + }, + NewItemsChunk { + previous: Some(ChunkIdentifier(1)), + new: ChunkIdentifier(2), + next: None, + }, + PushItems { at: Position(ChunkIdentifier(2), 0), items: vec!['l', 'm'] }, + NewGapChunk { + previous: Some(ChunkIdentifier(2)), + new: ChunkIdentifier(3), + next: None, + gap: (), + }, + ] + ); + + // Try to remove a gap that's not a gap. + let err = linked_chunk.remove_gap_at(ChunkIdentifier(0)).unwrap_err(); + assert_matches!(err, Error::ChunkIsItems { .. }); + + // Try to remove an unknown gap chunk. + let err = linked_chunk.remove_gap_at(ChunkIdentifier(42)).unwrap_err(); + assert_matches!(err, Error::InvalidChunkIdentifier { .. }); + + // Remove the gap in the middle. + let maybe_next = linked_chunk.remove_gap_at(ChunkIdentifier(1)).unwrap(); + let next = maybe_next.unwrap(); + // The next insert position at the start of the next chunk. + assert_eq!(next.chunk_identifier(), ChunkIdentifier(2)); + assert_eq!(next.index(), 0); + assert_items_eq!(linked_chunk, ['a', 'b'] ['l', 'm'] [-]); + assert_eq!(linked_chunk.updates().unwrap().take(), &[RemoveChunk(ChunkIdentifier(1))]); + + // Remove the gap at the end. + let next = linked_chunk.remove_gap_at(ChunkIdentifier(3)).unwrap(); + // It was the last chunk, so there's no next insert position. + assert!(next.is_none()); + assert_items_eq!(linked_chunk, ['a', 'b'] ['l', 'm']); + assert_eq!(linked_chunk.updates().unwrap().take(), &[RemoveChunk(ChunkIdentifier(3))]); + + Ok(()) + } + #[test] fn test_chunk_item_positions() { let mut linked_chunk = LinkedChunk::<3, char, ()>::new(); diff --git a/crates/matrix-sdk/src/event_cache/pagination.rs b/crates/matrix-sdk/src/event_cache/pagination.rs index 064acd5ae00..832398ec57c 100644 --- a/crates/matrix-sdk/src/event_cache/pagination.rs +++ b/crates/matrix-sdk/src/event_cache/pagination.rs @@ -178,12 +178,9 @@ impl RoomPagination { let insert_new_gap_pos = if let Some(gap_id) = prev_gap_id { // There is a prior gap, let's replace it by new events! trace!("replaced gap with new events from backpagination"); - Some( - room_events - .replace_gap_at(sync_events, gap_id) - .expect("gap_identifier is a valid chunk id we read previously") - .first_position(), - ) + room_events + .replace_gap_at(sync_events, gap_id) + .expect("gap_identifier is a valid chunk id we read previously") } else if let Some(pos) = first_event_pos { // No prior gap, but we had some events: assume we need to prepend events // before those. diff --git a/crates/matrix-sdk/src/event_cache/room/events.rs b/crates/matrix-sdk/src/event_cache/room/events.rs index 83fc2218972..67a65635153 100644 --- a/crates/matrix-sdk/src/event_cache/room/events.rs +++ b/crates/matrix-sdk/src/event_cache/room/events.rs @@ -145,13 +145,13 @@ impl RoomEvents { /// Because the `gap_identifier` can represent non-gap chunk, this method /// returns a `Result`. /// - /// This method returns a reference to the (first if many) newly created - /// `Chunk` that contains the `items`. + /// This method returns either the position of the first chunk that's been + /// created, or the next insert position if the chunk has been removed. pub fn replace_gap_at( &mut self, events: I, gap_identifier: ChunkIdentifier, - ) -> Result<&Chunk, Error> + ) -> Result, Error> where I: IntoIterator, { @@ -165,8 +165,14 @@ impl RoomEvents { // because of the removals. self.remove_events(duplicated_event_ids); - // Replace the gap by new events. - self.chunks.replace_gap_at(unique_events, gap_identifier) + if unique_events.is_empty() { + // There are no new events, so there's no need to create a new empty items + // chunk; instead, remove the gap. + self.chunks.remove_gap_at(gap_identifier) + } else { + // Replace the gap by new events. + Ok(Some(self.chunks.replace_gap_at(unique_events, gap_identifier)?.first_position())) + } } /// Search for a chunk, and return its identifier. @@ -711,9 +717,8 @@ mod tests { let chunk_identifier_of_gap = room_events .chunks() - .find_map(|chunk| chunk.is_gap().then_some(chunk.first_position())) - .unwrap() - .chunk_identifier(); + .find_map(|chunk| chunk.is_gap().then_some(chunk.identifier())) + .unwrap(); room_events.replace_gap_at([event_1, event_2], chunk_identifier_of_gap).unwrap(); @@ -752,9 +757,8 @@ mod tests { let chunk_identifier_of_gap = room_events .chunks() - .find_map(|chunk| chunk.is_gap().then_some(chunk.first_position())) - .unwrap() - .chunk_identifier(); + .find_map(|chunk| chunk.is_gap().then_some(chunk.identifier())) + .unwrap(); assert_events_eq!( room_events.events(), @@ -790,6 +794,40 @@ mod tests { } } + #[test] + fn test_replace_gap_at_with_no_new_events() { + let (_, event_0) = new_event("$ev0"); + let (_, event_1) = new_event("$ev1"); + let (_, event_2) = new_event("$ev2"); + + let mut room_events = RoomEvents::new(); + + room_events.push_events([event_0, event_1]); + room_events.push_gap(Gap { prev_token: "middle".to_owned() }); + room_events.push_events([event_2]); + room_events.push_gap(Gap { prev_token: "end".to_owned() }); + + // Remove the first gap. + let first_gap_id = room_events + .chunks() + .find_map(|chunk| chunk.is_gap().then_some(chunk.identifier())) + .unwrap(); + + // The next insert position is the next chunk's start. + let pos = room_events.replace_gap_at([], first_gap_id).unwrap(); + assert_eq!(pos, Some(Position::new(ChunkIdentifier::new(2), 0))); + + // Remove the second gap. + let second_gap_id = room_events + .chunks() + .find_map(|chunk| chunk.is_gap().then_some(chunk.identifier())) + .unwrap(); + + // No next insert position. + let pos = room_events.replace_gap_at([], second_gap_id).unwrap(); + assert!(pos.is_none()); + } + #[test] fn test_remove_events() { let (event_id_0, event_0) = new_event("$ev0"); diff --git a/crates/matrix-sdk/tests/integration/event_cache.rs b/crates/matrix-sdk/tests/integration/event_cache.rs index 0832fea4b5d..20541241840 100644 --- a/crates/matrix-sdk/tests/integration/event_cache.rs +++ b/crates/matrix-sdk/tests/integration/event_cache.rs @@ -14,7 +14,7 @@ use matrix_sdk::{ use matrix_sdk_test::{ async_test, event_factory::EventFactory, GlobalAccountDataTestEvent, JoinedRoomBuilder, }; -use ruma::{event_id, room_id, user_id}; +use ruma::{event_id, events::AnyTimelineEvent, room_id, serde::Raw, user_id}; use serde_json::json; use tokio::{spawn, sync::broadcast}; use wiremock::ResponseTemplate; @@ -916,3 +916,74 @@ async fn test_backpaginate_with_no_initial_events() { assert_event_matches_msg(&events[2], "world"); assert_eq!(events.len(), 3); } + +#[async_test] +async fn test_backpaginate_replace_empty_gap() { + let server = MatrixMockServer::new().await; + let client = server.client_builder().build().await; + + let event_cache = client.event_cache(); + + // Immediately subscribe the event cache to sync updates. + event_cache.subscribe().unwrap(); + + let room_id = room_id!("!omelette:fromage.fr"); + + let f = EventFactory::new().room(room_id).sender(user_id!("@a:b.c")); + + // Start with a room with an event, limited timeline and prev-batch token. + let room = server + .sync_room( + &client, + JoinedRoomBuilder::new(room_id) + .add_timeline_event(f.text_msg("world").event_id(event_id!("$2"))) + .set_timeline_limited() + .set_timeline_prev_batch("prev-batch".to_owned()), + ) + .await; + + let (room_event_cache, _drop_handles) = room.event_cache().await.unwrap(); + + let (events, mut stream) = room_event_cache.subscribe().await.unwrap(); + wait_for_initial_events(events, &mut stream).await; + + // The first back-pagination will return a previous-batch token, but no events. + server + .mock_room_messages() + .ok( + "start-token-unused1".to_owned(), + Some("prev_batch".to_owned()), + Vec::>::new(), + Vec::new(), + ) + .mock_once() + .mount() + .await; + + // The second round of back-pagination will return this one. + server + .mock_room_messages() + .from("prev_batch") + .ok( + "start-token-unused2".to_owned(), + None, + vec![f.text_msg("hello").event_id(event_id!("$1"))], + Vec::new(), + ) + .mock_once() + .mount() + .await; + + let pagination = room_event_cache.pagination(); + + // Run pagination twice. + pagination.run_backwards(20, once).await.unwrap(); + pagination.run_backwards(20, once).await.unwrap(); + + // The linked chunk should contain the events in the correct order. + let (events, _stream) = room_event_cache.subscribe().await.unwrap(); + + assert_event_matches_msg(&events[0], "hello"); + assert_event_matches_msg(&events[1], "world"); + assert_eq!(events.len(), 2); +}