Skip to content

Commit 17bc955

Browse files
HywanHywan
committed
task(sdk): Make the code more robust around event removals.
This patch makes the code more robust around event removals. Sorting events by their position is no longer done in the `Deduplicator` but in a new `RoomEventCacheState::remove_events` method, which removes events in the store and in the `RoomEvents`. This method is responsible to sort events, this stuff is less fragile like so.
1 parent 0b38fa1 commit 17bc955

File tree

3 files changed

+122
-106
lines changed

3 files changed

+122
-106
lines changed

‎crates/matrix-sdk/src/event_cache/deduplicator.rs‎

Lines changed: 8 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -91,17 +91,12 @@ impl Deduplicator {
9191
});
9292
}
9393

94-
let mut outcome = match self {
94+
match self {
9595
Deduplicator::InMemory(dedup) => Ok(dedup.filter_duplicate_events(events, room_events)),
9696
Deduplicator::PersistentStore(dedup) => {
9797
dedup.filter_duplicate_events(events, room_events).await
9898
}
99-
}?;
100-
101-
sort_events_by_position_descending(&mut outcome.in_memory_duplicated_event_ids);
102-
sort_events_by_position_descending(&mut outcome.in_store_duplicated_event_ids);
103-
104-
Ok(outcome)
99+
}
105100
}
106101
}
107102

@@ -329,23 +324,6 @@ pub(super) struct DeduplicationOutcome {
329324
pub in_store_duplicated_event_ids: Vec<(OwnedEventId, Position)>,
330325
}
331326

332-
/// Sort events so that they can be removed safely without messing their
333-
/// position.
334-
///
335-
/// This function sort events by their position if any.
336-
///
337-
/// Events must be sorted by their position index, from greatest to lowest, so
338-
/// that all positions remain valid inside the same chunk while they are being
339-
/// removed. For the sake of debugability, we also sort by position chunk
340-
/// identifier, but this is not required.
341-
fn sort_events_by_position_descending(event_ids: &mut [(OwnedEventId, Position)]) {
342-
event_ids.sort_by(|(_, a), (_, b)| {
343-
b.chunk_identifier()
344-
.cmp(&a.chunk_identifier())
345-
.then_with(|| a.index().cmp(&b.index()).reverse())
346-
});
347-
}
348-
349327
#[cfg(test)]
350328
mod tests {
351329
use assert_matches2::{assert_let, assert_matches};
@@ -363,36 +341,6 @@ mod tests {
363341
.into_event()
364342
}
365343

366-
#[test]
367-
fn test_sort_events_by_position_descending() {
368-
let ev1 = owned_event_id!("$ev1");
369-
let ev2 = owned_event_id!("$ev2");
370-
let ev3 = owned_event_id!("$ev3");
371-
let ev4 = owned_event_id!("$ev4");
372-
let ev5 = owned_event_id!("$ev5");
373-
374-
let mut event_ids = vec![
375-
(ev1.clone(), Position::new(ChunkIdentifier::new(2), 1)),
376-
(ev2.clone(), Position::new(ChunkIdentifier::new(1), 0)),
377-
(ev3.clone(), Position::new(ChunkIdentifier::new(2), 0)),
378-
(ev4.clone(), Position::new(ChunkIdentifier::new(1), 1)),
379-
(ev5.clone(), Position::new(ChunkIdentifier::new(0), 0)),
380-
];
381-
382-
sort_events_by_position_descending(&mut event_ids);
383-
384-
assert_eq!(
385-
event_ids,
386-
&[
387-
(ev1, Position::new(ChunkIdentifier::new(2), 1)),
388-
(ev3, Position::new(ChunkIdentifier::new(2), 0)),
389-
(ev4, Position::new(ChunkIdentifier::new(1), 1)),
390-
(ev2, Position::new(ChunkIdentifier::new(1), 0)),
391-
(ev5, Position::new(ChunkIdentifier::new(0), 0)),
392-
]
393-
);
394-
}
395-
396344
#[async_test]
397345
async fn test_filter_find_duplicates_in_the_input() {
398346
let event_id_0 = owned_event_id!("$ev0");
@@ -587,11 +535,11 @@ mod tests {
587535
assert_eq!(outcome.in_memory_duplicated_event_ids.len(), 2);
588536
assert_eq!(
589537
outcome.in_memory_duplicated_event_ids[0],
590-
(event_id_3, Position::new(ChunkIdentifier::new(0), 1))
538+
(event_id_2, Position::new(ChunkIdentifier::new(0), 0))
591539
);
592540
assert_eq!(
593541
outcome.in_memory_duplicated_event_ids[1],
594-
(event_id_2, Position::new(ChunkIdentifier::new(0), 0))
542+
(event_id_3, Position::new(ChunkIdentifier::new(0), 1))
595543
);
596544

597545
// From these 4 events, 2 are duplicated and live in the store only, they have
@@ -601,11 +549,11 @@ mod tests {
601549
assert_eq!(outcome.in_store_duplicated_event_ids.len(), 2);
602550
assert_eq!(
603551
outcome.in_store_duplicated_event_ids[0],
604-
(event_id_1, Position::new(ChunkIdentifier::new(42), 1))
552+
(event_id_0, Position::new(ChunkIdentifier::new(42), 0))
605553
);
606554
assert_eq!(
607555
outcome.in_store_duplicated_event_ids[1],
608-
(event_id_0, Position::new(ChunkIdentifier::new(42), 0))
556+
(event_id_1, Position::new(ChunkIdentifier::new(42), 1))
609557
);
610558
}
611559

@@ -755,11 +703,11 @@ mod tests {
755703
assert_eq!(in_store_duplicated_event_ids.len(), 2);
756704
assert_eq!(
757705
in_store_duplicated_event_ids[0],
758-
(eid2.to_owned(), Position::new(ChunkIdentifier::new(43), 0))
706+
(eid1.to_owned(), Position::new(ChunkIdentifier::new(42), 0))
759707
);
760708
assert_eq!(
761709
in_store_duplicated_event_ids[1],
762-
(eid1.to_owned(), Position::new(ChunkIdentifier::new(42), 0))
710+
(eid2.to_owned(), Position::new(ChunkIdentifier::new(43), 0))
763711
);
764712
}
765713
}

‎crates/matrix-sdk/src/event_cache/room/events.rs‎

Lines changed: 49 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -253,14 +253,10 @@ impl RoomEvents {
253253

254254
/// Remove some events from the linked chunk.
255255
///
256-
/// Empty chunks are going to be removed.
257-
///
258-
/// Events **must** be sorted by their position (descending, i.e. from
259-
/// newest to oldest).
260-
pub fn remove_events_by_position<P>(&mut self, positions: P) -> Result<(), Error>
261-
where
262-
P: Iterator<Item = Position>,
263-
{
256+
/// If a chunk becomes empty, it's going to be removed.
257+
pub fn remove_events_by_position(&mut self, mut positions: Vec<Position>) -> Result<(), Error> {
258+
sort_positions_descending(&mut positions);
259+
264260
for position in positions {
265261
self.chunks.remove_item_at(
266262
position,
@@ -385,6 +381,21 @@ fn chunk_debug_string(content: &ChunkContent<Event, Gap>) -> String {
385381
}
386382
}
387383

384+
/// Sort positions of events so that events can be removed safely without
385+
/// messing their position.
386+
///
387+
/// Events must be sorted by their position index, from greatest to lowest, so
388+
/// that all positions remain valid inside the same chunk while they are being
389+
/// removed. For the sake of debugability, we also sort by position chunk
390+
/// identifier, but this is not required.
391+
pub(super) fn sort_positions_descending(positions: &mut [Position]) {
392+
positions.sort_by(|a, b| {
393+
b.chunk_identifier()
394+
.cmp(&a.chunk_identifier())
395+
.then_with(|| a.index().cmp(&b.index()).reverse())
396+
});
397+
}
398+
388399
#[cfg(test)]
389400
mod tests {
390401
use assert_matches::assert_matches;
@@ -658,13 +669,10 @@ mod tests {
658669

659670
// Remove some events.
660671
room_events
661-
.remove_events_by_position(
662-
[
663-
Position::new(ChunkIdentifier::new(2), 1),
664-
Position::new(ChunkIdentifier::new(0), 1),
665-
]
666-
.into_iter(),
667-
)
672+
.remove_events_by_position(vec![
673+
Position::new(ChunkIdentifier::new(2), 1),
674+
Position::new(ChunkIdentifier::new(0), 1),
675+
])
668676
.unwrap();
669677

670678
assert_events_eq!(
@@ -677,7 +685,7 @@ mod tests {
677685

678686
// Ensure chunks are removed once empty.
679687
room_events
680-
.remove_events_by_position([Position::new(ChunkIdentifier::new(2), 0)].into_iter())
688+
.remove_events_by_position(vec![Position::new(ChunkIdentifier::new(2), 0)])
681689
.unwrap();
682690

683691
assert_events_eq!(
@@ -699,7 +707,7 @@ mod tests {
699707
// Remove one undefined event.
700708
// An error is expected.
701709
room_events
702-
.remove_events_by_position([Position::new(ChunkIdentifier::new(42), 153)].into_iter())
710+
.remove_events_by_position(vec![Position::new(ChunkIdentifier::new(42), 153)])
703711
.unwrap_err();
704712

705713
assert_events_eq!(room_events.events(), []);
@@ -781,4 +789,28 @@ mod tests {
781789
assert_eq!(&output[0], "chunk #0: events[$12345678, $2]");
782790
assert_eq!(&output[1], "chunk #1: gap['raclette']");
783791
}
792+
793+
#[test]
794+
fn test_sort_positions_descending() {
795+
let mut positions = vec![
796+
Position::new(ChunkIdentifier::new(2), 1),
797+
Position::new(ChunkIdentifier::new(1), 0),
798+
Position::new(ChunkIdentifier::new(2), 0),
799+
Position::new(ChunkIdentifier::new(1), 1),
800+
Position::new(ChunkIdentifier::new(0), 0),
801+
];
802+
803+
sort_positions_descending(&mut positions);
804+
805+
assert_eq!(
806+
positions,
807+
&[
808+
Position::new(ChunkIdentifier::new(2), 1),
809+
Position::new(ChunkIdentifier::new(2), 0),
810+
Position::new(ChunkIdentifier::new(1), 1),
811+
Position::new(ChunkIdentifier::new(1), 0),
812+
Position::new(ChunkIdentifier::new(0), 0),
813+
]
814+
);
815+
}
784816
}

0 commit comments

Comments
 (0)