From ea6e15086e81092b9988bf6a732fe9125678a4de Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 22 May 2024 10:16:02 +0200 Subject: [PATCH] feat(sdk): `as_vector`s are no longer unsafe. This patch removes the `unsafe` part of `as_vector`. The idea is to pass `Iter` (the forward iterator of `Chunk`) to `AsVector` so that it internally computes `initial_chunk_lengths`. The shape of this data must no longer be guaranteed by the caller. This patch goes a bit further: `UpdateToVectorDiff` has a new constructor which consumes this `Iter` and builds `initial_chunk_lengths` itself. Even better! Finally, `Updates::as_vector` is removed. It's clearly no longer necessary and it was creating borrowing issues anyway with the new code structure. --- .../src/event_cache/linked_chunk/as_vector.rs | 45 ++++++++++--------- .../src/event_cache/linked_chunk/mod.rs | 23 +++------- .../src/event_cache/linked_chunk/updates.rs | 17 ++----- 3 files changed, 35 insertions(+), 50 deletions(-) diff --git a/crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs b/crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs index bd403fc53..c48f8b35f 100644 --- a/crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs +++ b/crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs @@ -22,21 +22,18 @@ use eyeball_im::VectorDiff; use super::{ updates::{ReaderToken, Update, UpdatesInner}, - ChunkIdentifier, + ChunkContent, ChunkIdentifier, Iter, }; /// A type alias to represent a chunk's length. This is purely for commodity. -pub(super) type ChunkLength = usize; +type ChunkLength = usize; /// A type that transforms a `Vec>` (given by /// [`Updates`](super::Update)) into a `Vec>` (this /// type). Basically, it helps to consume a [`LinkedChunk`] as /// if it was an [`eyeball::ObservableVector`]. pub struct AsVector { - /// Weak reference to [`UpdatesInner`]. - /// - /// Using a weak reference allows [`Updates`] to be dropped - /// freely even if a subscriber exists. + /// Strong reference to [`UpdatesInner`]. updates: Arc>>, /// The token to read the updates. @@ -48,22 +45,12 @@ pub struct AsVector { impl AsVector { /// Create a new [`Self`]. - /// - /// `initial_chunk_lengths` must be pairs of all chunk identifiers with the - /// associated chunk length. The pairs must be in the exact same order than - /// in [`LinkedChunk`]. - /// - /// # Safety - /// - /// This method is marked as unsafe only to attract the caller's attention - /// on the order of pairs. If the order of pairs are incorrect, the entire - /// algorithm will not work properly and is very likely to panic. - pub(super) unsafe fn new( + pub(super) fn new( updates: Arc>>, token: ReaderToken, - initial_chunk_lengths: VecDeque<(ChunkIdentifier, ChunkLength)>, + chunk_iterator: Iter, ) -> Self { - Self { updates, token, mapper: UpdateToVectorDiff { chunks: initial_chunk_lengths } } + Self { updates, token, mapper: UpdateToVectorDiff::new(chunk_iterator) } } /// Take the new updates as [`VectorDiff`]. @@ -89,6 +76,23 @@ struct UpdateToVectorDiff { } impl UpdateToVectorDiff { + /// Construct [`Self`]. + fn new(chunk_iterator: Iter) -> Self { + let mut initial_chunk_lengths = VecDeque::new(); + + for chunk in chunk_iterator { + initial_chunk_lengths.push_front(( + chunk.identifier(), + match chunk.content() { + ChunkContent::Gap(_) => 0, + ChunkContent::Items(items) => items.len(), + }, + )) + } + + Self { chunks: initial_chunk_lengths } + } + /// Map several [`Update`] into [`VectorDiff`]. /// /// How does this type transform `Update` into `VectorDiff`? There is no @@ -97,7 +101,8 @@ impl UpdateToVectorDiff { /// computed manually. /// /// The only buffered data is pairs of [`ChunkIdentifier`] and - /// [`ChunkLength`]. The following rules must be respected: + /// [`ChunkLength`]. The following rules must be respected (they are defined + /// in [`Self::new`]): /// /// * A chunk of kind [`ChunkContent::Gap`] has a length of 0, /// * A chunk of kind [`ChunkContent::Items`] has a length equals to its diff --git a/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs b/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs index 9f5374bab..fa4a49eef 100644 --- a/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs +++ b/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs @@ -95,7 +95,6 @@ mod as_vector; mod updates; use std::{ - collections::VecDeque, fmt, marker::PhantomData, ops::Not, @@ -742,23 +741,13 @@ impl LinkedChunk { /// been constructed with [`Self::new`], otherwise, if it's been constructed /// with [`Self::new_with_update_history`], it returns `Some(…)`. pub fn as_vector(&mut self) -> Option> { - let mut initial_chunk_lengths = VecDeque::new(); + let (updates, token) = self + .updates + .as_mut() + .map(|updates| (updates.inner.clone(), updates.new_reader_token()))?; + let chunk_iterator = self.chunks(); - for chunk in self.chunks() { - initial_chunk_lengths.push_front(( - chunk.identifier(), - match chunk.content() { - ChunkContent::Gap(_) => 0, - ChunkContent::Items(items) => items.len(), - }, - )) - } - - self.updates.as_mut().map(|updates| { - // SAFETY: The order of chunk pairs inside `initial_chunk_lengths` is exactly - // the same as chunks in `Self`. - unsafe { updates.as_vector(initial_chunk_lengths) } - }) + Some(AsVector::new(updates, token, chunk_iterator)) } } diff --git a/crates/matrix-sdk/src/event_cache/linked_chunk/updates.rs b/crates/matrix-sdk/src/event_cache/linked_chunk/updates.rs index bd711d994..2ca602aa2 100644 --- a/crates/matrix-sdk/src/event_cache/linked_chunk/updates.rs +++ b/crates/matrix-sdk/src/event_cache/linked_chunk/updates.rs @@ -13,7 +13,7 @@ // limitations under the License. use std::{ - collections::{HashMap, VecDeque}, + collections::HashMap, pin::Pin, sync::{Arc, RwLock, Weak}, task::{Context, Poll, Waker}, @@ -21,7 +21,7 @@ use std::{ use futures_core::Stream; -use super::{AsVector, ChunkIdentifier, ChunkLength, Position}; +use super::{ChunkIdentifier, Position}; /// Represent the updates that have happened inside a [`LinkedChunk`]. /// @@ -120,7 +120,7 @@ where /// /// Get a value for this type with [`LinkedChunk::updates`]. pub struct Updates { - inner: Arc>>, + pub(super) inner: Arc>>, } /// A token used to represent readers that read the updates in @@ -279,16 +279,7 @@ impl Updates { UpdatesSubscriber::new(Arc::downgrade(&self.inner), token) } - pub(super) unsafe fn as_vector( - &mut self, - initial_chunk_lengths: VecDeque<(ChunkIdentifier, ChunkLength)>, - ) -> AsVector { - // An `AsVector` is a new update reader, it needs its own token. - let token = self.new_reader_token(); - - AsVector::new(Arc::clone(&self.inner), token, initial_chunk_lengths) - } - + /// Generate a new [`ReaderToken`]. pub(super) fn new_reader_token(&mut self) -> ReaderToken { let mut inner = self.inner.write().unwrap();