From 1018d71bb793aa01f3b7a81eb6589f358074cdb3 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 10 Oct 2024 17:05:35 +0200 Subject: [PATCH] refactor(event cache): get rid of the `RoomPaginationData` data structure It only contained two fields, and it avoids one extra level of cognitive overhead and makes the type hierarchy flatter. --- crates/matrix-sdk/src/event_cache/mod.rs | 17 ++++++----- .../matrix-sdk/src/event_cache/pagination.rs | 29 +++++-------------- 2 files changed, 17 insertions(+), 29 deletions(-) diff --git a/crates/matrix-sdk/src/event_cache/mod.rs b/crates/matrix-sdk/src/event_cache/mod.rs index db0d6a9f1..391c6d710 100644 --- a/crates/matrix-sdk/src/event_cache/mod.rs +++ b/crates/matrix-sdk/src/event_cache/mod.rs @@ -39,7 +39,7 @@ use matrix_sdk_base::{ sync::{JoinedRoomUpdate, LeftRoomUpdate, RoomUpdates, Timeline}, }; use matrix_sdk_common::executor::{spawn, JoinHandle}; -use paginator::PaginatorState; +use paginator::{Paginator, PaginatorState}; use ruma::{ events::{ relation::RelationType, @@ -52,12 +52,11 @@ use ruma::{ }; use tokio::sync::{ broadcast::{error::RecvError, Receiver, Sender}, - Mutex, RwLock, RwLockReadGuard, RwLockWriteGuard, + Mutex, Notify, RwLock, RwLockReadGuard, RwLockWriteGuard, }; use tracing::{error, info_span, instrument, trace, warn, Instrument as _, Span}; use self::{ - pagination::RoomPaginationData, paginator::PaginatorError, store::{Gap, RoomEvents}, }; @@ -634,6 +633,9 @@ struct RoomEventCacheInner { /// [`RoomEventCacheInner`] instances. all_events: Arc>, + /// A notifier that we received a new pagination token. + pub pagination_batch_token_notifier: Notify, + /// A paginator instance, that's configured to run back-pagination on our /// behalf. /// @@ -642,7 +644,7 @@ struct RoomEventCacheInner { /// events received from those kinds of pagination with the cache. This /// paginator is only used for queries that interact with the actual event /// cache. - pagination: RoomPaginationData, + pub paginator: Paginator, } impl RoomEventCacheInner { @@ -665,7 +667,8 @@ impl RoomEventCacheInner { }), all_events: all_events_cache, sender, - pagination: RoomPaginationData::new(weak_room), + pagination_batch_token_notifier: Default::default(), + paginator: Paginator::new(weak_room), } } @@ -788,7 +791,7 @@ impl RoomEventCacheInner { .await?; // Reset the paginator status to initial. - self.pagination.paginator.set_idle_state(PaginatorState::Initial, prev_batch, None)?; + self.paginator.set_idle_state(PaginatorState::Initial, prev_batch, None)?; Ok(()) } @@ -923,7 +926,7 @@ impl RoomEventCacheInner { // Now that all events have been added, we can trigger the // `pagination_token_notifier`. if prev_batch.is_some() { - self.pagination.token_notifier.notify_one(); + self.pagination_batch_token_notifier.notify_one(); } // The order of `RoomEventCacheUpdate`s is **really** important here. diff --git a/crates/matrix-sdk/src/event_cache/pagination.rs b/crates/matrix-sdk/src/event_cache/pagination.rs index ce0a8e3dd..035093711 100644 --- a/crates/matrix-sdk/src/event_cache/pagination.rs +++ b/crates/matrix-sdk/src/event_cache/pagination.rs @@ -18,31 +18,16 @@ use std::{future::Future, ops::ControlFlow, sync::Arc, time::Duration}; use eyeball::Subscriber; use matrix_sdk_base::deserialized_responses::SyncTimelineEvent; -use tokio::{sync::Notify, time::timeout}; +use tokio::time::timeout; use tracing::{debug, instrument, trace}; use super::{ - paginator::{PaginableRoom, PaginationResult, Paginator, PaginatorState}, + paginator::{PaginationResult, PaginatorState}, store::Gap, BackPaginationOutcome, Result, RoomEventCacheInner, }; use crate::event_cache::{linked_chunk::ChunkContent, store::RoomEvents}; -#[derive(Debug)] -pub(super) struct RoomPaginationData { - /// A notifier that we received a new pagination token. - pub token_notifier: Notify, - - /// The stateful paginator instance used for the integrated pagination. - pub paginator: Paginator, -} - -impl RoomPaginationData { - pub(super) fn new(room: PR) -> Self { - Self { token_notifier: Default::default(), paginator: Paginator::new(room) } - } -} - /// An API object to run pagination queries on a [`super::RoomEventCache`]. /// /// Can be created with [`super::RoomEventCache::pagination()`]. @@ -135,7 +120,7 @@ impl RoomPagination { let prev_token = self.get_or_wait_for_token(Some(DEFAULT_WAIT_FOR_TOKEN_DURATION)).await; - let paginator = &self.inner.pagination.paginator; + let paginator = &self.inner.paginator; paginator.set_idle_state(PaginatorState::Idle, prev_token.clone(), None)?; @@ -282,7 +267,7 @@ impl RoomPagination { // Otherwise, wait for a notification that we received a previous-batch token. // Note the state lock is released while doing so, allowing other tasks to write // into the linked chunk. - let _ = timeout(wait_time, self.inner.pagination.token_notifier.notified()).await; + let _ = timeout(wait_time, self.inner.pagination_batch_token_notifier.notified()).await; let mut state = self.inner.state.write().await; let token = get_oldest(&state.events); @@ -293,7 +278,7 @@ impl RoomPagination { /// Returns a subscriber to the pagination status used for the /// back-pagination integrated to the event cache. pub fn status(&self) -> Subscriber { - self.inner.pagination.paginator.state() + self.inner.paginator.state() } /// Returns whether we've hit the start of the timeline. @@ -301,7 +286,7 @@ impl RoomPagination { /// This is true if, and only if, we didn't have a previous-batch token and /// running backwards pagination would be useless. pub fn hit_timeline_start(&self) -> bool { - self.inner.pagination.paginator.hit_timeline_start() + self.inner.paginator.hit_timeline_start() } /// Returns whether we've hit the end of the timeline. @@ -309,7 +294,7 @@ impl RoomPagination { /// This is true if, and only if, we didn't have a next-batch token and /// running forwards pagination would be useless. pub fn hit_timeline_end(&self) -> bool { - self.inner.pagination.paginator.hit_timeline_end() + self.inner.paginator.hit_timeline_end() } }