Skip to content

Commit

Permalink
feat(event cache): don't replace a gap chunk by an empty items chunks
Browse files Browse the repository at this point in the history
bnjbvr committed Dec 17, 2024
1 parent 5d8ad3a commit 373709f
Showing 4 changed files with 246 additions and 35 deletions.
139 changes: 122 additions & 17 deletions crates/matrix-sdk-common/src/linked_chunk/mod.rs
Original file line number Diff line number Diff line change
@@ -634,6 +634,47 @@ impl<const CAP: usize, Item, Gap> LinkedChunk<CAP, Item, Gap> {
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<Option<Position>, 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<const CAP: usize, Item, Gap> LinkedChunk<CAP, Item, Gap> {
.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();
9 changes: 3 additions & 6 deletions crates/matrix-sdk/src/event_cache/pagination.rs
Original file line number Diff line number Diff line change
@@ -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.
60 changes: 49 additions & 11 deletions crates/matrix-sdk/src/event_cache/room/events.rs
Original file line number Diff line number Diff line change
@@ -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<I>(
&mut self,
events: I,
gap_identifier: ChunkIdentifier,
) -> Result<&Chunk<DEFAULT_CHUNK_CAPACITY, Event, Gap>, Error>
) -> Result<Option<Position>, Error>
where
I: IntoIterator<Item = Event>,
{
@@ -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");
73 changes: 72 additions & 1 deletion crates/matrix-sdk/tests/integration/event_cache.rs
Original file line number Diff line number Diff line change
@@ -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::<Raw<AnyTimelineEvent>>::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);
}

0 comments on commit 373709f

Please sign in to comment.