From 2f769726dda4789e3b155816498d2093769f883a Mon Sep 17 00:00:00 2001 From: Julian Sparber Date: Fri, 12 Mar 2021 17:02:25 +0100 Subject: [PATCH 1/3] matrix-sdk: prevent dupplicated members requests --- matrix_sdk/src/client.rs | 15 +++++++------- matrix_sdk/src/room/common.rs | 38 +++++++++++++++++++++++++++-------- 2 files changed, 38 insertions(+), 15 deletions(-) diff --git a/matrix_sdk/src/client.rs b/matrix_sdk/src/client.rs index 0639a635..0b7d06c5 100644 --- a/matrix_sdk/src/client.rs +++ b/matrix_sdk/src/client.rs @@ -88,16 +88,15 @@ use matrix_sdk_common::{ }; #[cfg(feature = "encryption")] -use matrix_sdk_common::{ - api::r0::{ - keys::{get_keys, upload_keys, upload_signing_keys::Request as UploadSigningKeysRequest}, - to_device::send_event_to_device::{ - Request as RumaToDeviceRequest, Response as ToDeviceResponse, - }, +use matrix_sdk_common::api::r0::{ + keys::{get_keys, upload_keys, upload_signing_keys::Request as UploadSigningKeysRequest}, + to_device::send_event_to_device::{ + Request as RumaToDeviceRequest, Response as ToDeviceResponse, }, - locks::Mutex, }; +use matrix_sdk_common::locks::Mutex; + use crate::{ error::HttpError, http_client::{client_with_config, HttpClient, HttpSend}, @@ -138,6 +137,7 @@ pub struct Client { #[cfg(feature = "encryption")] /// Lock making sure we're only doing one key claim request at a time. key_claim_lock: Arc>, + pub(crate) members_request_locks: DashMap>>, } #[cfg(not(tarpaulin_include))] @@ -393,6 +393,7 @@ impl Client { group_session_locks: DashMap::new(), #[cfg(feature = "encryption")] key_claim_lock: Arc::new(Mutex::new(())), + members_request_locks: DashMap::new(), }) } diff --git a/matrix_sdk/src/room/common.rs b/matrix_sdk/src/room/common.rs index aa4fd222..2a3fa773 100644 --- a/matrix_sdk/src/room/common.rs +++ b/matrix_sdk/src/room/common.rs @@ -2,7 +2,9 @@ use matrix_sdk_common::api::r0::{ membership::{get_member_events, join_room_by_id, leave_room}, message::get_message_events, }; -use std::ops::Deref; +use matrix_sdk_common::locks::Mutex; + +use std::{ops::Deref, sync::Arc}; use crate::{Client, Result, Room, RoomMember}; @@ -96,14 +98,34 @@ impl Common { } pub(crate) async fn request_members(&self) -> Result<()> { - // TODO: don't send a request if a request is being sent - let request = get_member_events::Request::new(self.inner.room_id()); - let response = self.client.send(request, None).await?; + #[allow(clippy::map_clone)] + if let Some(mutex) = self + .client + .members_request_locks + .get(self.inner.room_id()) + .map(|m| m.clone()) + { + mutex.lock().await; + } else { + let mutex = Arc::new(Mutex::new(())); + self.client + .members_request_locks + .insert(self.inner.room_id().clone(), mutex.clone()); - self.client - .base_client - .receive_members(self.inner.room_id(), &response) - .await?; + let _guard = mutex.lock().await; + + let request = get_member_events::Request::new(self.inner.room_id()); + let response = self.client.send(request, None).await?; + + self.client + .base_client + .receive_members(self.inner.room_id(), &response) + .await?; + + self.client + .members_request_locks + .remove(self.inner.room_id()); + } Ok(()) } From 5465a7b5119ed5f74e23b5a557b1598e63935b68 Mon Sep 17 00:00:00 2001 From: Julian Sparber Date: Fri, 12 Mar 2021 20:18:43 +0100 Subject: [PATCH 2/3] matrix-sdk: prevent frequent typing_notice requests --- matrix_sdk/src/client.rs | 10 +++--- matrix_sdk/src/room/joined.rs | 58 +++++++++++++++++++++++++++-------- 2 files changed, 50 insertions(+), 18 deletions(-) diff --git a/matrix_sdk/src/client.rs b/matrix_sdk/src/client.rs index 0b7d06c5..a601312d 100644 --- a/matrix_sdk/src/client.rs +++ b/matrix_sdk/src/client.rs @@ -138,6 +138,7 @@ pub struct Client { /// Lock making sure we're only doing one key claim request at a time. key_claim_lock: Arc>, pub(crate) members_request_locks: DashMap>>, + pub(crate) typing_notice_times: DashMap, } #[cfg(not(tarpaulin_include))] @@ -394,6 +395,7 @@ impl Client { #[cfg(feature = "encryption")] key_claim_lock: Arc::new(Mutex::new(())), members_request_locks: DashMap::new(), + typing_notice_times: DashMap::new(), }) } @@ -1669,7 +1671,6 @@ impl Client { /// # use std::{path::PathBuf, time::Duration}; /// # use matrix_sdk::{ /// # Client, SyncSettings, - /// # api::r0::typing::create_typing_event::Typing, /// # identifiers::room_id, /// # }; /// # use futures::executor::block_on; @@ -1747,7 +1748,6 @@ impl Client { /// # use std::{path::PathBuf, time::Duration}; /// # use matrix_sdk::{ /// # Client, SyncSettings, - /// # api::r0::typing::create_typing_event::Typing, /// # identifiers::room_id, /// # }; /// # use futures::executor::block_on; @@ -1802,7 +1802,7 @@ mod test { api::r0::{ account::register::Request as RegistrationRequest, directory::get_public_rooms_filtered::Request as PublicRoomsFilterRequest, - membership::Invite3pid, typing::create_typing_event::Typing, uiaa::AuthData, + membership::Invite3pid, uiaa::AuthData, }, assign, directory::Filter, @@ -2451,9 +2451,7 @@ mod test { .get_joined_room(&room_id!("!SVkFJHzfwvuaIEawgC:localhost")) .unwrap(); - room.typing_notice(Typing::Yes(std::time::Duration::from_secs(1))) - .await - .unwrap(); + room.typing_notice(true).await.unwrap(); } #[tokio::test] diff --git a/matrix_sdk/src/room/joined.rs b/matrix_sdk/src/room/joined.rs index 42cb8fb3..e4ee82b5 100644 --- a/matrix_sdk/src/room/joined.rs +++ b/matrix_sdk/src/room/joined.rs @@ -26,6 +26,7 @@ use matrix_sdk_common::{ AnyMessageEventContent, AnyStateEventContent, }, identifiers::{EventId, UserId}, + instant::{Duration, Instant}, uuid::Uuid, }; @@ -40,6 +41,9 @@ use matrix_sdk_base::crypto::AttachmentEncryptor; #[cfg(feature = "encryption")] use tracing::instrument; +const TYPING_NOTICE_TIMEOUT: Duration = Duration::from_secs(4); +const TYPING_NOTICE_RESEND_TIMEOUT: Duration = Duration::from_secs(3); + /// A room in the joined state. /// /// The `JoinedRoom` contains all methodes specific to a `Room` with type `RoomType::Joined`. @@ -137,11 +141,16 @@ impl Joined { Ok(()) } - /// Send a request to notify this room of a user typing. + /// Activate typing notice for this room. + /// + /// The typing notice remains active for 4s. It can be deactivate at any point by setting + /// typing to `false`. If this method is called while the typing notice is active nothing will happen. + /// This method can be called on every key stroke, since it will do nothing while typing is + /// active. /// /// # Arguments /// - /// * `typing` - Whether the user is typing, and how long. + /// * `typing` - Whether the user is typing or has stopped typing. /// /// # Examples /// @@ -149,7 +158,6 @@ impl Joined { /// # use std::time::Duration; /// # use matrix_sdk::{ /// # Client, SyncSettings, - /// # api::r0::typing::create_typing_event::Typing, /// # identifiers::room_id, /// # }; /// # use futures::executor::block_on; @@ -162,21 +170,47 @@ impl Joined { /// # .get_joined_room(&room_id!("!SVkFJHzfwvuaIEawgC:localhost")) /// # .unwrap(); /// # room - /// .typing_notice(Typing::Yes(Duration::from_secs(4))) + /// .typing_notice(true) /// .await /// .expect("Can't get devices from server"); /// # }); /// /// ``` - pub async fn typing_notice(&self, typing: impl Into) -> Result<()> { - // TODO: don't send a request if a typing notice is being sent or is already active - let request = TypingRequest::new( - self.inner.own_user_id(), - self.inner.room_id(), - typing.into(), - ); + pub async fn typing_notice(&self, typing: bool) -> Result<()> { + // Only send a request to the homeserver if the old timeout has elapsed or the typing + // notice changed state within the TYPING_NOTICE_TIMEOUT + let send = + if let Some(typing_time) = self.client.typing_notice_times.get(self.inner.room_id()) { + if typing_time.elapsed() > TYPING_NOTICE_RESEND_TIMEOUT { + // We always reactivate the typing notice if typing is true or we may need to + // deactivate it if it's currently active if typing is false + typing || typing_time.elapsed() <= TYPING_NOTICE_TIMEOUT + } else { + // Only send a request when we need to deactivate typing + !typing + } + } else { + // Typing notice is currently deactivated, therefore, send a request only when it's + // about to be activated + typing + }; + + if send { + let typing = if typing { + self.client + .typing_notice_times + .insert(self.inner.room_id().clone(), Instant::now()); + Typing::Yes(TYPING_NOTICE_TIMEOUT) + } else { + self.client.typing_notice_times.remove(self.inner.room_id()); + Typing::No + }; + + let request = + TypingRequest::new(self.inner.own_user_id(), self.inner.room_id(), typing); + self.client.send(request, None).await?; + } - self.client.send(request, None).await?; Ok(()) } From 387104e6e00a5bdfa76a697c4068e778d8e0d099 Mon Sep 17 00:00:00 2001 From: Julian Sparber Date: Tue, 16 Mar 2021 16:37:50 +0100 Subject: [PATCH 3/3] matrix-sdk: wrap request locks into an Arc --- matrix_sdk/src/client.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/matrix_sdk/src/client.rs b/matrix_sdk/src/client.rs index a601312d..d5595dd6 100644 --- a/matrix_sdk/src/client.rs +++ b/matrix_sdk/src/client.rs @@ -133,12 +133,12 @@ pub struct Client { /// Locks making sure we only have one group session sharing request in /// flight per room. #[cfg(feature = "encryption")] - pub(crate) group_session_locks: DashMap>>, + pub(crate) group_session_locks: Arc>>>, #[cfg(feature = "encryption")] /// Lock making sure we're only doing one key claim request at a time. key_claim_lock: Arc>, - pub(crate) members_request_locks: DashMap>>, - pub(crate) typing_notice_times: DashMap, + pub(crate) members_request_locks: Arc>>>, + pub(crate) typing_notice_times: Arc>, } #[cfg(not(tarpaulin_include))] @@ -391,11 +391,11 @@ impl Client { http_client, base_client, #[cfg(feature = "encryption")] - group_session_locks: DashMap::new(), + group_session_locks: Arc::new(DashMap::new()), #[cfg(feature = "encryption")] key_claim_lock: Arc::new(Mutex::new(())), - members_request_locks: DashMap::new(), - typing_notice_times: DashMap::new(), + members_request_locks: Arc::new(DashMap::new()), + typing_notice_times: Arc::new(DashMap::new()), }) }