From 5e755c5d514a23a987cf30291daaf1ac9a31efa9 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 13 Jun 2024 14:39:39 +0200 Subject: [PATCH 1/3] fix(ui): `room_list_service::Room::new` no longer receives a `SlidingSyncRoom`. This patch updates `room_list_service::Room::new` to take a `&Client` and a `&RoomId` instead of a `SlidingSyncRoom`. The `SlidingSyncRoom` is only used in `Room::default_room_timeline_builder` and is fetched there from the `SlidingSync` instance lazily. It confines the `SlidingSyncRoom` to one single method for `Room` now. --- .../src/room_list_service/mod.rs | 5 +-- .../src/room_list_service/room.rs | 36 ++++++++++--------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list_service/mod.rs b/crates/matrix-sdk-ui/src/room_list_service/mod.rs index 7c6a35b155b..48f1f0c7dc2 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/mod.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/mod.rs @@ -433,10 +433,7 @@ impl RoomListService { } } - let room = match self.sliding_sync.get_room(room_id).await { - Some(room) => Room::new(self.sliding_sync.clone(), room)?, - None => return Err(Error::RoomNotFound(room_id.to_owned())), - }; + let room = Room::new(&self.client, room_id, &self.sliding_sync)?; self.rooms.write().await.push(room.clone()); diff --git a/crates/matrix-sdk-ui/src/room_list_service/room.rs b/crates/matrix-sdk-ui/src/room_list_service/room.rs index 9d16ba8184c..52a70a4b803 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/room.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/room.rs @@ -17,7 +17,7 @@ use std::{ops::Deref, sync::Arc}; use async_once_cell::OnceCell as AsyncOnceCell; -use matrix_sdk::{event_cache, SlidingSync, SlidingSyncRoom}; +use matrix_sdk::{Client, SlidingSync}; use ruma::{api::client::sync::sync_events::v4::RoomSubscription, events::StateEventType, RoomId}; use super::Error; @@ -39,9 +39,6 @@ struct RoomInner { /// The Sliding Sync where everything comes from. sliding_sync: Arc, - /// The Sliding Sync room. - sliding_sync_room: SlidingSyncRoom, - /// The underlying client room. room: matrix_sdk::Room, @@ -60,18 +57,16 @@ impl Deref for Room { impl Room { /// Create a new `Room`. pub(super) fn new( - sliding_sync: Arc, - sliding_sync_room: SlidingSyncRoom, + client: &Client, + room_id: &RoomId, + sliding_sync: &Arc, ) -> Result { - let room = sliding_sync_room - .client() - .get_room(sliding_sync_room.room_id()) - .ok_or_else(|| Error::RoomNotFound(sliding_sync_room.room_id().to_owned()))?; + let room = + client.get_room(room_id).ok_or_else(|| Error::RoomNotFound(room_id.to_owned()))?; Ok(Self { inner: Arc::new(RoomInner { - sliding_sync, - sliding_sync_room, + sliding_sync: sliding_sync.clone(), room, timeline: AsyncOnceCell::new(), }), @@ -185,18 +180,27 @@ impl Room { } /// Create a new [`TimelineBuilder`] with the default configuration. - pub async fn default_room_timeline_builder(&self) -> event_cache::Result { + pub async fn default_room_timeline_builder(&self) -> Result { // TODO we can remove this once the event cache handles his own cache. + + let sliding_sync_room = + self.inner + .sliding_sync + .get_room(self.inner.room.room_id()) + .await + .ok_or_else(|| Error::RoomNotFound(self.inner.room.room_id().to_owned()))?; + self.inner .room .client() .event_cache() .add_initial_events( self.inner.room.room_id(), - self.inner.sliding_sync_room.timeline_queue().iter().cloned().collect(), - self.inner.sliding_sync_room.prev_batch(), + sliding_sync_room.timeline_queue().iter().cloned().collect(), + sliding_sync_room.prev_batch(), ) - .await?; + .await + .map_err(Error::EventCache)?; Ok(Timeline::builder(&self.inner.room).track_read_marker_and_receipts()) } From edec6e755801dabe816abef4c21bfedc2773c885 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 13 Jun 2024 14:47:43 +0200 Subject: [PATCH 2/3] feat(ui): `RoomListService::room` is no longer async. This patch makes `RoomListService::room` synchronous. It no longer reads a `SlidingSyncRoom` from `SlidingSync`, then it not needs to be async anymore. This patch replaces the `RwLock` of `RoomListService::rooms` from `tokio::sync` to `std::sync`. The patch updates all calls to `RoomListService::room` to remove the `.await` point. --- bindings/matrix-sdk-ffi/src/room_list.rs | 10 +++++----- .../matrix-sdk-ui/src/room_list_service/mod.rs | 18 ++++++++++-------- .../tests/integration/room_list_service.rs | 16 ++++++++-------- labs/multiverse/src/main.rs | 2 +- 4 files changed, 24 insertions(+), 22 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/room_list.rs b/bindings/matrix-sdk-ffi/src/room_list.rs index acfd70026eb..8b875173b84 100644 --- a/bindings/matrix-sdk-ffi/src/room_list.rs +++ b/bindings/matrix-sdk-ffi/src/room_list.rs @@ -125,11 +125,11 @@ impl RoomListService { }))) } - async fn room(&self, room_id: String) -> Result, RoomListError> { + fn room(&self, room_id: String) -> Result, RoomListError> { let room_id = <&RoomId>::try_from(room_id.as_str()).map_err(RoomListError::from)?; Ok(Arc::new(RoomListItem { - inner: Arc::new(self.inner.room(room_id).await?), + inner: Arc::new(self.inner.room(room_id)?), utd_hook: self.utd_hook.clone(), })) } @@ -172,7 +172,7 @@ pub struct RoomList { inner: Arc, } -#[uniffi::export(async_runtime = "tokio")] +#[uniffi::export] impl RoomList { fn loading_state( &self, @@ -233,8 +233,8 @@ impl RoomList { } } - async fn room(&self, room_id: String) -> Result, RoomListError> { - self.room_list_service.room(room_id).await + fn room(&self, room_id: String) -> Result, RoomListError> { + self.room_list_service.room(room_id) } } diff --git a/crates/matrix-sdk-ui/src/room_list_service/mod.rs b/crates/matrix-sdk-ui/src/room_list_service/mod.rs index 48f1f0c7dc2..4861cc9fd01 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/mod.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/mod.rs @@ -66,7 +66,12 @@ mod room; mod room_list; mod state; -use std::{future::ready, num::NonZeroUsize, sync::Arc, time::Duration}; +use std::{ + future::ready, + num::NonZeroUsize, + sync::{Arc, RwLock}, + time::Duration, +}; use async_stream::stream; use eyeball::{SharedObservable, Subscriber}; @@ -90,10 +95,7 @@ use ruma::{ }; pub use state::*; use thiserror::Error; -use tokio::{ - sync::{Mutex, RwLock}, - time::timeout, -}; +use tokio::{sync::Mutex, time::timeout}; use crate::timeline; @@ -424,9 +426,9 @@ impl RoomListService { } /// Get a [`Room`] if it exists. - pub async fn room(&self, room_id: &RoomId) -> Result { + pub fn room(&self, room_id: &RoomId) -> Result { { - let rooms = self.rooms.read().await; + let rooms = self.rooms.read().unwrap(); if let Some(room) = rooms.iter().rfind(|room| room.id() == room_id) { return Ok(room.clone()); @@ -435,7 +437,7 @@ impl RoomListService { let room = Room::new(&self.client, room_id, &self.sliding_sync)?; - self.rooms.write().await.push(room.clone()); + self.rooms.write().unwrap().push(room.clone()); Ok(room) } diff --git a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs index 8981f704714..8333916aec1 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs @@ -2072,7 +2072,7 @@ async fn test_room() -> Result<(), Error> { }, }; - let room0 = room_list.room(room_id_0).await?; + let room0 = room_list.room(room_id_0)?; // Room has received a name from sliding sync. assert_eq!(room0.cached_display_name(), Some("Room #0".to_owned())); @@ -2080,7 +2080,7 @@ async fn test_room() -> Result<(), Error> { // Room has received an avatar from sliding sync. assert_eq!(room0.avatar_url(), Some(mxc_uri!("mxc://homeserver/media").to_owned())); - let room1 = room_list.room(room_id_1).await?; + let room1 = room_list.room(room_id_1)?; // Room has not received a name from sliding sync, then it's calculated. assert_eq!(room1.cached_display_name(), Some("Empty Room".to_owned())); @@ -2144,7 +2144,7 @@ async fn test_room_not_found() -> Result<(), Error> { let room_id = room_id!("!foo:bar.org"); assert_matches!( - room_list.room(room_id).await, + room_list.room(room_id), Err(Error::RoomNotFound(error_room_id)) => { assert_eq!(error_room_id, room_id.to_owned()); } @@ -2208,7 +2208,7 @@ async fn test_room_subscription() -> Result<(), Error> { }, }; - let room1 = room_list.room(room_id_1).await.unwrap(); + let room1 = room_list.room(room_id_1).unwrap(); // Subscribe. @@ -2253,7 +2253,7 @@ async fn test_room_subscription() -> Result<(), Error> { // Unsubscribe. room1.unsubscribe(); - room_list.room(room_id_2).await?.unsubscribe(); // unsubscribe from a room that has no subscription. + room_list.room(room_id_2)?.unsubscribe(); // unsubscribe from a room that has no subscription. sync_then_assert_request_and_fake_response! { [server, room_list, sync] @@ -2316,7 +2316,7 @@ async fn test_room_unread_notifications() -> Result<(), Error> { }, }; - let room = room_list.room(room_id).await.unwrap(); + let room = room_list.room(room_id).unwrap(); assert_matches!( room.unread_notification_counts(), @@ -2391,7 +2391,7 @@ async fn test_room_timeline() -> Result<(), Error> { }, }; - let room = room_list.room(room_id).await?; + let room = room_list.room(room_id)?; room.init_timeline_with_builder(room.default_room_timeline_builder().await.unwrap()).await?; let timeline = room.timeline().unwrap(); @@ -2473,7 +2473,7 @@ async fn test_room_latest_event() -> Result<(), Error> { }, }; - let room = room_list.room(room_id).await?; + let room = room_list.room(room_id)?; room.init_timeline_with_builder(room.default_room_timeline_builder().await.unwrap()).await?; // The latest event does not exist. diff --git a/labs/multiverse/src/main.rs b/labs/multiverse/src/main.rs index 84396636d54..cf58d2b7168 100644 --- a/labs/multiverse/src/main.rs +++ b/labs/multiverse/src/main.rs @@ -227,7 +227,7 @@ impl App { all_rooms.into_iter().filter(|room_id| !previous_ui_rooms.contains_key(room_id)) { // Retrieve the room list service's Room. - let Ok(ui_room) = sync_service.room_list_service().room(&room_id).await else { + let Ok(ui_room) = sync_service.room_list_service().room(&room_id) else { error!("error when retrieving room after an update"); continue; }; From a7ff0587a675c1aa7d08180f73e57cab860090f4 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 13 Jun 2024 15:03:59 +0200 Subject: [PATCH 3/3] fix(ui): Replace the `RwLock` by a `Mutex` in `RoomListService::rooms`. This patch replaces the `RwLock` by a `Mutex` in `RoomListService::rooms` because: 1. it removes a race condition, 2. `RwLock` was used because the lock could have been taken for a long time due to the previous `.await` point. It's no longer the case, so we can blindly use a `Mutex` here. --- .../matrix-sdk-ui/src/room_list_service/mod.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list_service/mod.rs b/crates/matrix-sdk-ui/src/room_list_service/mod.rs index 4861cc9fd01..50fa5137012 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/mod.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/mod.rs @@ -69,7 +69,7 @@ mod state; use std::{ future::ready, num::NonZeroUsize, - sync::{Arc, RwLock}, + sync::{Arc, Mutex as StdMutex}, time::Duration, }; @@ -114,7 +114,7 @@ pub struct RoomListService { state: SharedObservable, /// Room cache, to avoid recreating `Room`s every time users fetch them. - rooms: Arc>>, + rooms: Arc>>, /// The current viewport ranges. /// @@ -204,7 +204,7 @@ impl RoomListService { client, sliding_sync, state: SharedObservable::new(State::Init), - rooms: Arc::new(RwLock::new(RingBuffer::new(Self::ROOM_OBJECT_CACHE_SIZE))), + rooms: Arc::new(StdMutex::new(RingBuffer::new(Self::ROOM_OBJECT_CACHE_SIZE))), viewport_ranges: Mutex::new(vec![VISIBLE_ROOMS_DEFAULT_RANGE]), }) } @@ -427,17 +427,16 @@ impl RoomListService { /// Get a [`Room`] if it exists. pub fn room(&self, room_id: &RoomId) -> Result { - { - let rooms = self.rooms.read().unwrap(); + let mut rooms = self.rooms.lock().unwrap(); - if let Some(room) = rooms.iter().rfind(|room| room.id() == room_id) { - return Ok(room.clone()); - } + if let Some(room) = rooms.iter().rfind(|room| room.id() == room_id) { + return Ok(room.clone()); } let room = Room::new(&self.client, room_id, &self.sliding_sync)?; - self.rooms.write().unwrap().push(room.clone()); + // Save for later. + rooms.push(room.clone()); Ok(room) }