From 9183f5d4ef2236adcc1f2ecff57b4f837d054ff6 Mon Sep 17 00:00:00 2001 From: Jonas Platte Date: Wed, 24 Aug 2022 22:51:51 +0200 Subject: [PATCH] refactor(sdk): Move event handler fields from Client into a new struct --- crates/matrix-sdk/src/client/builder.rs | 2 - crates/matrix-sdk/src/client/mod.rs | 43 ++------------ crates/matrix-sdk/src/event_handler.rs | 78 ++++++++++++++++++++----- 3 files changed, 69 insertions(+), 54 deletions(-) diff --git a/crates/matrix-sdk/src/client/builder.rs b/crates/matrix-sdk/src/client/builder.rs index 1c84e4472..c35e59681 100644 --- a/crates/matrix-sdk/src/client/builder.rs +++ b/crates/matrix-sdk/src/client/builder.rs @@ -427,8 +427,6 @@ impl ClientBuilder { members_request_locks: Default::default(), typing_notice_times: Default::default(), event_handlers: Default::default(), - event_handler_data: Default::default(), - event_handler_counter: Default::default(), notification_handlers: Default::default(), appservice_mode: self.appservice_mode, respect_login_well_known: self.respect_login_well_known, diff --git a/crates/matrix-sdk/src/client/mod.rs b/crates/matrix-sdk/src/client/mod.rs index 7a7522bde..7a606c53c 100644 --- a/crates/matrix-sdk/src/client/mod.rs +++ b/crates/matrix-sdk/src/client/mod.rs @@ -15,15 +15,13 @@ // limitations under the License. use std::{ - collections::{btree_map, BTreeMap}, fmt::{self, Debug}, future::Future, io::Read, pin::Pin, - sync::{atomic::AtomicU64, Arc, RwLock as StdRwLock, RwLockReadGuard as StdRwLockReadGuard}, + sync::Arc, }; -use anymap2::any::CloneAnySendSync; #[cfg(target_arch = "wasm32")] use async_once_cell::OnceCell; use dashmap::DashMap; @@ -95,8 +93,7 @@ use crate::{ config::RequestConfig, error::{HttpError, HttpResult}, event_handler::{ - EventHandler, EventHandlerHandle, EventHandlerKey, EventHandlerResult, EventHandlerWrapper, - SyncEvent, + EventHandler, EventHandlerHandle, EventHandlerResult, EventHandlerStore, SyncEvent, }, http_client::HttpClient, room, Account, Error, RefreshTokenError, Result, RumaApiError, @@ -117,8 +114,6 @@ const DEFAULT_UPLOAD_SPEED: u64 = 125_000; /// 5 min minimal upload request timeout, used to clamp the request timeout. const MIN_UPLOAD_REQUEST_TIMEOUT: Duration = Duration::from_secs(60 * 5); -type EventHandlerMap = BTreeMap>; - #[cfg(not(target_arch = "wasm32"))] type NotificationHandlerFut = Pin + Send>>; #[cfg(target_arch = "wasm32")] @@ -131,8 +126,6 @@ type NotificationHandlerFn = type NotificationHandlerFn = Box NotificationHandlerFut>; -type AnyMap = anymap2::Map; - /// Enum controlling if a loop running callbacks should continue or abort. /// /// This is mainly used in the [`sync_with_callback`] method, the return value @@ -176,12 +169,7 @@ pub(crate) struct ClientInner { pub(crate) members_request_locks: DashMap>>, pub(crate) typing_notice_times: DashMap, /// Event handlers. See `add_event_handler`. - pub(crate) event_handlers: StdRwLock, - /// Custom event handler context. See `add_event_handler_context`. - pub(crate) event_handler_data: StdRwLock, - /// When registering a event handler, the current value is used for the - /// handlers identification, then the counter is incremented. - pub(crate) event_handler_counter: AtomicU64, + pub(crate) event_handlers: EventHandlerStore, /// Notification handlers. See `register_notification_handler`. notification_handlers: RwLock>, /// Whether the client should operate in application service style mode. @@ -667,10 +655,6 @@ impl Client { self } - pub(crate) fn event_handlers(&self) -> StdRwLockReadGuard<'_, EventHandlerMap> { - self.inner.event_handlers.read().unwrap() - } - /// Remove the event handler associated with the handle. /// /// Note that you **must not** call `remove_event_handler` from the @@ -730,16 +714,7 @@ impl Client { /// # }); /// ``` pub fn remove_event_handler(&self, handle: EventHandlerHandle) { - let mut event_handlers = self.inner.event_handlers.write().unwrap(); - - if let btree_map::Entry::Occupied(mut entry) = event_handlers.entry(handle.key) { - let v = entry.get_mut(); - v.retain(|e| e.handler_id != handle.handler_id); - - if v.is_empty() { - entry.remove(); - } - } + self.inner.event_handlers.remove(handle); } /// Add an arbitrary value for use as event handler context. @@ -787,7 +762,7 @@ impl Client { where T: Clone + Send + Sync + 'static, { - self.inner.event_handler_data.write().unwrap().insert(ctx); + self.inner.event_handlers.add_context(ctx); } #[allow(missing_docs)] @@ -800,14 +775,6 @@ impl Client { self } - pub(crate) fn event_handler_context(&self) -> Option - where - T: Clone + Send + Sync + 'static, - { - let map = self.inner.event_handler_data.read().unwrap(); - map.get::().cloned() - } - /// Register a handler for a notification. /// /// Similar to [`Client::add_event_handler`], but only allows functions diff --git a/crates/matrix-sdk/src/event_handler.rs b/crates/matrix-sdk/src/event_handler.rs index 9b54d481f..f8edb07da 100644 --- a/crates/matrix-sdk/src/event_handler.rs +++ b/crates/matrix-sdk/src/event_handler.rs @@ -35,14 +35,19 @@ use std::any::TypeId; use std::{ borrow::{Borrow, Cow}, + collections::{btree_map, BTreeMap}, fmt, future::Future, iter, ops::Deref, pin::Pin, - sync::atomic::Ordering::SeqCst, + sync::{ + atomic::{AtomicU64, Ordering::SeqCst}, + RwLock, RwLockReadGuard, + }, }; +use anymap2::any::CloneAnySendSync; use matrix_sdk_base::{ deserialized_responses::{EncryptionInfo, SyncRoomEvent}, SendOutsideWasm, SyncOutsideWasm, @@ -64,6 +69,56 @@ type EventHandlerFn = dyn Fn(EventHandlerData<'_>) -> EventHandlerFut + Send + S #[cfg(target_arch = "wasm32")] type EventHandlerFn = dyn Fn(EventHandlerData<'_>) -> EventHandlerFut; +type EventHandlerMap = BTreeMap>; +type AnyMap = anymap2::Map; + +#[derive(Default)] +pub(crate) struct EventHandlerStore { + handlers: RwLock, + context: RwLock, + counter: AtomicU64, +} + +impl EventHandlerStore { + pub fn add_handler( + &self, + key: EventHandlerKey, + handler_id: u64, + handler_fn: Box, + ) { + self.handlers + .write() + .unwrap() + .entry(key) + .or_default() + .push(EventHandlerWrapper { handler_id, handler_fn }); + } + + pub fn add_context(&self, ctx: T) + where + T: Clone + Send + Sync + 'static, + { + self.context.write().unwrap().insert(ctx); + } + + pub fn remove(&self, handle: EventHandlerHandle) { + let mut map = self.handlers.write().unwrap(); + + if let btree_map::Entry::Occupied(mut entry) = map.entry(handle.key) { + let v = entry.get_mut(); + v.retain(|e| e.handler_id != handle.handler_id); + + if v.is_empty() { + entry.remove(); + } + } + } + + fn read(&self) -> RwLockReadGuard<'_, EventHandlerMap> { + self.handlers.read().unwrap() + } +} + #[doc(hidden)] #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] pub enum EventKind { @@ -275,7 +330,8 @@ pub struct Ctx(pub T); impl EventHandlerContext for Ctx { fn from_data(data: &EventHandlerData<'_>) -> Option { - data.client.event_handler_context::().map(Ctx) + let map = data.client.inner.event_handlers.context.read().unwrap(); + map.get::().cloned().map(Ctx) } } @@ -361,16 +417,10 @@ impl Client { }) }); - let handler_id = self.inner.event_handler_counter.fetch_add(1, SeqCst); + let handler_id = self.inner.event_handlers.counter.fetch_add(1, SeqCst); let key = EventHandlerKey::new(Ev::KIND, Ev::TYPE, room_id); - self.inner - .event_handlers - .write() - .unwrap() - .entry(key.clone()) - .or_default() - .push(EventHandlerWrapper { handler_fn, handler_id }); + self.inner.event_handlers.add_handler(key.clone(), handler_id, handler_fn); EventHandlerHandle { key, handler_id } } @@ -496,7 +546,7 @@ impl Client { EventHandlerKeyInner { ev_kind, ev_type, room_id } }); - let handlers_lock = self.event_handlers(); + let handlers_lock = self.inner.event_handlers.read(); iter::once(non_room_handler_key) .chain(room_handler_key) @@ -950,15 +1000,15 @@ mod tests { let client = no_retry_test_client(None).await; let handle = client.add_event_handler(|_ev: OriginalSyncRoomMemberEvent| async {}); - assert_eq!(client.event_handlers().len(), 1); + assert_eq!(client.inner.event_handlers.read().len(), 1); { let _guard = client.event_handler_drop_guard(handle); - assert_eq!(client.event_handlers().len(), 1); + assert_eq!(client.inner.event_handlers.read().len(), 1); // guard dropped here } - assert_eq!(client.event_handlers().len(), 0); + assert_eq!(client.inner.event_handlers.read().len(), 0); } #[async_test]