From e21d1fcb937405387a6887c3c4a050c7787f0e2b Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 24 Apr 2023 14:11:12 +0200 Subject: [PATCH] feat(sdk): `SlidingSyncList` no longer implement `Clone`. It's not possible to clone a `SlidingSyncList` anymore. Why? Because it's not correct. Prior to this patch, it was possible to add a list to a `SlidingSync` instance, then add a clone of the same list to another `SlidingSync` instance. Weird behaviors could happen, but more importantly, for the next Sliding Sync design we are working on, it could lead to gigantic bugs. Removing `Clone` from `SlidingSyncList` makes the code simpler. For example, `SlidingSyncList.inner` no longer needs an `Arc`, or `SlidingSync::stream` no longer needs to clone all the lists. --- crates/matrix-sdk/src/sliding_sync/builder.rs | 2 +- .../src/sliding_sync/list/builder.rs | 4 +-- .../matrix-sdk/src/sliding_sync/list/mod.rs | 17 +++------- crates/matrix-sdk/src/sliding_sync/mod.rs | 34 +++++-------------- 4 files changed, 17 insertions(+), 40 deletions(-) diff --git a/crates/matrix-sdk/src/sliding_sync/builder.rs b/crates/matrix-sdk/src/sliding_sync/builder.rs index 0eea4760e..f7cc05514 100644 --- a/crates/matrix-sdk/src/sliding_sync/builder.rs +++ b/crates/matrix-sdk/src/sliding_sync/builder.rs @@ -24,7 +24,7 @@ use crate::{Client, Result}; /// /// Get a new builder with methods like [`crate::Client::sliding_sync`], or /// [`crate::SlidingSync::builder`]. -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct SlidingSyncBuilder { storage_key: Option, homeserver: Option, diff --git a/crates/matrix-sdk/src/sliding_sync/list/builder.rs b/crates/matrix-sdk/src/sliding_sync/list/builder.rs index 713e8ae95..fcdaf92d3 100644 --- a/crates/matrix-sdk/src/sliding_sync/list/builder.rs +++ b/crates/matrix-sdk/src/sliding_sync/list/builder.rs @@ -156,7 +156,7 @@ impl SlidingSyncListBuilder { }; Ok(SlidingSyncList { - inner: Arc::new(SlidingSyncListInner { + inner: SlidingSyncListInner { // From the builder sync_mode: self.sync_mode, sort: self.sort, @@ -173,7 +173,7 @@ impl SlidingSyncListBuilder { state: StdRwLock::new(Observable::new(SlidingSyncState::default())), maximum_number_of_rooms: StdRwLock::new(Observable::new(None)), room_list: StdRwLock::new(ObservableVector::new()), - }), + }, }) } } diff --git a/crates/matrix-sdk/src/sliding_sync/list/mod.rs b/crates/matrix-sdk/src/sliding_sync/list/mod.rs index e8da8aeab..fe8bfa5a1 100644 --- a/crates/matrix-sdk/src/sliding_sync/list/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/list/mod.rs @@ -3,14 +3,7 @@ mod frozen; mod request_generator; mod room_list_entry; -use std::{ - cmp::min, - collections::HashSet, - fmt::Debug, - iter, - ops::Not, - sync::{Arc, RwLock as StdRwLock}, -}; +use std::{cmp::min, collections::HashSet, fmt::Debug, iter, ops::Not, sync::RwLock as StdRwLock}; pub use builder::*; use eyeball::unique::Observable; @@ -30,9 +23,9 @@ use crate::Result; /// Holding a specific filtered list within the concept of sliding sync. /// /// It is OK to clone this type as much as you need: cloning it is cheap. -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct SlidingSyncList { - inner: Arc, + inner: SlidingSyncListInner, } impl SlidingSyncList { @@ -985,7 +978,7 @@ mod tests { #[test] fn test_sliding_sync_list_new_builder() { let list = SlidingSyncList { - inner: Arc::new(SlidingSyncListInner { + inner: SlidingSyncListInner { sync_mode: SlidingSyncMode::Growing, sort: vec!["foo".to_owned(), "bar".to_owned()], required_state: vec![(StateEventType::RoomName, "baz".to_owned())], @@ -1002,7 +995,7 @@ mod tests { 42, Some(153), )), - }), + }, }; let new_list = list.new_builder().build().unwrap(); diff --git a/crates/matrix-sdk/src/sliding_sync/mod.rs b/crates/matrix-sdk/src/sliding_sync/mod.rs index 1936d633d..c3587c314 100644 --- a/crates/matrix-sdk/src/sliding_sync/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/mod.rs @@ -23,7 +23,6 @@ mod list; mod room; use std::{ - borrow::BorrowMut, collections::BTreeMap, fmt::Debug, mem, @@ -188,12 +187,8 @@ impl SlidingSync { } /// Get access to the SlidingSyncList named `list_name`. - /// - /// Note: Remember that this list might have been changed since you started - /// listening to the stream and is therefor not necessarily up to date - /// with the lists used for the stream. - pub fn list(&self, list_name: &str) -> Option { - self.inner.lists.read().unwrap().get(list_name).cloned() + pub fn list(&self, _list_name: &str) -> Option { + todo!("this is going to be removed!"); } /// Add the list to the list of lists. @@ -201,10 +196,6 @@ impl SlidingSync { /// As lists need to have a unique `.name`, if a list with the same name /// is found the new list will replace the old one and the return it or /// `None`. - /// - /// Note: Remember that this change will only be applicable for any new - /// stream created after this. The old stream will still continue to use the - /// previous set of lists. pub fn add_list(&self, list: SlidingSyncList) -> Option { self.inner.lists.write().unwrap().insert(list.name().to_owned(), list) } @@ -267,12 +258,11 @@ impl SlidingSync { } /// Handle the HTTP response. - #[instrument(skip_all, fields(lists = lists.len()))] + #[instrument(skip_all, fields(lists = self.inner.lists.read().unwrap().len()))] fn handle_response( &self, sliding_sync_response: v4::Response, mut sync_response: SyncResponse, - lists: &mut BTreeMap, ) -> Result { { debug!( @@ -328,6 +318,8 @@ impl SlidingSync { // Update the lists. let mut updated_lists = Vec::with_capacity(sliding_sync_response.lists.len()); + let mut lists = self.inner.lists.write().unwrap(); + for (name, updates) in sliding_sync_response.lists { let Some(list) = lists.get_mut(&name) else { error!("Response for list `{name}` - unknown to us; skipping"); @@ -354,16 +346,11 @@ impl SlidingSync { Ok(update_summary) } - async fn sync_once( - &self, - stream_id: &str, - lists: Arc>>, - ) -> Result> { + async fn sync_once(&self, stream_id: &str) -> Result> { let mut requests_lists = BTreeMap::new(); { - let mut lists_lock = lists.lock().unwrap(); - let lists = lists_lock.borrow_mut(); + let mut lists = self.inner.lists.write().unwrap(); if lists.is_empty() { return Ok(None); @@ -487,7 +474,7 @@ impl SlidingSync { debug!(?sync_response, "Sliding Sync response has been handled by the client"); - let updates = this.handle_response(response, sync_response, lists.lock().unwrap().borrow_mut())?; + let updates = this.handle_response(response, sync_response)?; this.cache_to_storage().await?; @@ -507,9 +494,6 @@ impl SlidingSync { #[allow(unknown_lints, clippy::let_with_type_underscore)] // triggered by instrument macro #[instrument(name = "sync_stream", skip_all)] pub fn stream(&self) -> impl Stream> + '_ { - // Copy all the lists. - let lists = Arc::new(Mutex::new(self.inner.lists.read().unwrap().clone())); - // Define a stream ID. let stream_id = Uuid::new_v4().to_string(); @@ -525,7 +509,7 @@ impl SlidingSync { debug!(?self.inner.extensions, "Sync stream loop is running"); }); - match self.sync_once(&stream_id, lists.clone()).instrument(sync_span.clone()).await { + match self.sync_once(&stream_id).instrument(sync_span.clone()).await { Ok(Some(updates)) => { self.inner.reset_counter.store(0, Ordering::SeqCst);