From a8a1315cb7cc6a8d58cf1fad56269772630fb304 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Fri, 12 Jul 2024 12:41:08 +0200 Subject: [PATCH] feat(sdk): Remove the `experimental-not-simplified-sliding-sync` feature flag. This patch is the second over two patches to change the support of Simplified MSC3575 from a compiler feature flag to a runtime configuration (see previous patch for the first one). First off, this patch removes all uses of the `experimental-not-simplified-sliding-sync` feature flag. Second, this patch uses the `Client::is_simplified_sliding_sync_enabled` method to know whether Simplified MSC3575 `Request`s must be transformed into MSC3575 `Request`s and vice versa for the `Response`s. To achieve that, the `SlidingSync::sync_once` method moves most of its body into a new inner private `send` method that takes a generic for the `Request` because now it can be `http::msc3575::Request` or a `http::simplified_msc3575::Request`. This `send` function has a specific bound for the `Request::IncomingResponse`: `http::Into::`, so that we are sure we can get back a Simplified MSC3575 `Response` in all cases. To make it work in both cases, this patch adds a `From for Response` implementation in `http::simplified_msc3575`. --- bindings/matrix-sdk-ffi/Cargo.toml | 2 - .../matrix-sdk-base/src/sliding_sync/http.rs | 6 + crates/matrix-sdk/Cargo.toml | 1 - crates/matrix-sdk/src/sliding_sync/mod.rs | 236 ++++++++++-------- labs/multiverse/Cargo.toml | 2 +- .../matrix-sdk-integration-testing/Cargo.toml | 6 +- 6 files changed, 140 insertions(+), 113 deletions(-) diff --git a/bindings/matrix-sdk-ffi/Cargo.toml b/bindings/matrix-sdk-ffi/Cargo.toml index c31015f104b..d954bfb302f 100644 --- a/bindings/matrix-sdk-ffi/Cargo.toml +++ b/bindings/matrix-sdk-ffi/Cargo.toml @@ -57,7 +57,6 @@ features = [ "e2e-encryption", "experimental-oidc", "experimental-sliding-sync", - "experimental-not-simplified-sliding-sync", # we don't want simplified MSC3575 yet! "experimental-widgets", "markdown", "rustls-tls", # note: differ from block below @@ -73,7 +72,6 @@ features = [ "e2e-encryption", "experimental-oidc", "experimental-sliding-sync", - "experimental-not-simplified-sliding-sync", # we don't want simplified MSC3575 yet! "experimental-widgets", "markdown", "native-tls", # note: differ from block above diff --git a/crates/matrix-sdk-base/src/sliding_sync/http.rs b/crates/matrix-sdk-base/src/sliding_sync/http.rs index fa210fa5523..7d96e782d77 100644 --- a/crates/matrix-sdk-base/src/sliding_sync/http.rs +++ b/crates/matrix-sdk-base/src/sliding_sync/http.rs @@ -266,6 +266,12 @@ pub mod simplified_msc3575 { use super::{msc3575 as ss, From}; + impl From for Response { + fn from(value: Response) -> Self { + value + } + } + impl From for Response { fn from(value: ss::Response) -> Self { let mut s = Self::new(value.pos); diff --git a/crates/matrix-sdk/Cargo.toml b/crates/matrix-sdk/Cargo.toml index 04469541420..53484b31c9f 100644 --- a/crates/matrix-sdk/Cargo.toml +++ b/crates/matrix-sdk/Cargo.toml @@ -59,7 +59,6 @@ experimental-sliding-sync = [ "matrix-sdk-base/experimental-sliding-sync", "reqwest/gzip", ] -experimental-not-simplified-sliding-sync = ["experimental-sliding-sync"] experimental-widgets = ["dep:language-tags", "dep:uuid"] docsrs = ["e2e-encryption", "sqlite", "indexeddb", "sso-login", "qrcode", "image-proc"] diff --git a/crates/matrix-sdk/src/sliding_sync/mod.rs b/crates/matrix-sdk/src/sliding_sync/mod.rs index 78c898e5615..91afbd17040 100644 --- a/crates/matrix-sdk/src/sliding_sync/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/mod.rs @@ -36,7 +36,10 @@ use async_stream::stream; use futures_core::stream::Stream; pub use matrix_sdk_base::sliding_sync::http; use matrix_sdk_common::{ring_buffer::RingBuffer, timer}; -use ruma::{api::client::error::ErrorKind, assign, OwnedEventId, OwnedRoomId, RoomId}; +use ruma::{ + api::{client::error::ErrorKind, OutgoingRequest}, + assign, OwnedEventId, OwnedRoomId, RoomId, +}; use serde::{Deserialize, Serialize}; use tokio::{ select, spawn, @@ -536,127 +539,152 @@ impl SlidingSync { #[instrument(skip_all, fields(pos))] async fn sync_once(&self) -> Result { - let (request, request_config, mut position_guard) = + let (request, request_config, position_guard) = self.generate_sync_request(&mut LazyTransactionId::new()).await?; // The code manipulates `Request` and `Response` from Simplified MSC3575 - // because it's simpler. If the `experimental-not-simplified-sliding-sync` - // feature flag is turned on, we transform the Simplified MSC3575 `Request` - // into a MSC3575 `Request`. - #[cfg(feature = "experimental-not-simplified-sliding-sync")] - let request: http::msc3575::Request = http::From::from(request); - - debug!("Sending request"); - - // Prepare the request. - let request = - self.inner.client.send(request, Some(request_config)).with_homeserver_override( - self.inner.sliding_sync_proxy.as_ref().map(ToString::to_string), - ); + // because it's the future standard. If + // `Client::is_simplified_sliding_sync_enabled` is turned off, the + // Simplified MSC3575 `Request` must be transformed into a MSC3575 `Request`. + return if !self.inner.client.is_simplified_sliding_sync_enabled() { + send( + self, + http::Into::::into(request), + request_config, + position_guard, + ) + .await + } else { + send(self, request, request_config, position_guard).await + }; - // Send the request and get a response with end-to-end encryption support. - // - // Sending the `/sync` request out when end-to-end encryption is enabled means - // that we need to also send out any outgoing e2ee related request out - // coming from the `OlmMachine::outgoing_requests()` method. + // The sending logic. It takes a generic `Request` because it can be a + // Simplified MSC3575 or a MSC3575 `Request`. + async fn send( + this: &SlidingSync, + request: Request, + request_config: RequestConfig, + mut position_guard: OwnedMutexGuard, + ) -> Result + where + Request: OutgoingRequest + Clone + Debug + Send + Sync + 'static, + Request::IncomingResponse: Send + + Sync + + + // This is required to get back a Simplified MSC3575 `Response` whatever the + // `Request` type. + http::Into, + crate::HttpError: From>, + { + debug!("Sending request"); - #[cfg(feature = "e2e-encryption")] - let response = { - if self.is_e2ee_enabled() { - // Here, we need to run 2 things: - // - // 1. Send the sliding sync request and get a response, - // 2. Send the E2EE requests. - // - // We don't want to use a `join` or `try_join` because we want to fail if and - // only if sending the sliding sync request fails. Failing to send the E2EE - // requests should just result in a log. - // - // We also want to give the priority to sliding sync request. E2EE requests are - // sent concurrently to the sliding sync request, but the priority is on waiting - // a sliding sync response. - // - // If sending sliding sync request fails, the sending of E2EE requests must be - // aborted as soon as possible. + // Prepare the request. + let request = + this.inner.client.send(request, Some(request_config)).with_homeserver_override( + this.inner.sliding_sync_proxy.as_ref().map(ToString::to_string), + ); - let client = self.inner.client.clone(); - let e2ee_uploads = spawn(async move { - if let Err(error) = client.send_outgoing_requests().await { - error!(?error, "Error while sending outgoing E2EE requests"); - } - }) - // Ensure that the task is not running in detached mode. It is aborted when it's - // dropped. - .abort_on_drop(); - - // Wait on the sliding sync request success or failure early. - let response = request.await?; - - // At this point, if `request` has been resolved successfully, we wait on - // `e2ee_uploads`. It did run concurrently, so it should not be blocking for too - // long. Otherwise —if `request` has failed— `e2ee_uploads` has - // been dropped, so aborted. - e2ee_uploads.await.map_err(|error| Error::JoinError { - task_description: "e2ee_uploads".to_owned(), - error, - })?; - - response - } else { - request.await? - } - }; + // Send the request and get a response with end-to-end encryption support. + // + // Sending the `/sync` request out when end-to-end encryption is enabled means + // that we need to also send out any outgoing e2ee related request out + // coming from the `OlmMachine::outgoing_requests()` method. - // Send the request and get a response _without_ end-to-end encryption support. - #[cfg(not(feature = "e2e-encryption"))] - let response = request.await?; + #[cfg(feature = "e2e-encryption")] + let response = { + if this.is_e2ee_enabled() { + // Here, we need to run 2 things: + // + // 1. Send the sliding sync request and get a response, + // 2. Send the E2EE requests. + // + // We don't want to use a `join` or `try_join` because we want to fail if and + // only if sending the sliding sync request fails. Failing to send the E2EE + // requests should just result in a log. + // + // We also want to give the priority to sliding sync request. E2EE requests are + // sent concurrently to the sliding sync request, but the priority is on waiting + // a sliding sync response. + // + // If sending sliding sync request fails, the sending of E2EE requests must be + // aborted as soon as possible. + + let client = this.inner.client.clone(); + let e2ee_uploads = spawn(async move { + if let Err(error) = client.send_outgoing_requests().await { + error!(?error, "Error while sending outgoing E2EE requests"); + } + }) + // Ensure that the task is not running in detached mode. It is aborted when it's + // dropped. + .abort_on_drop(); + + // Wait on the sliding sync request success or failure early. + let response = request.await?; + + // At this point, if `request` has been resolved successfully, we wait on + // `e2ee_uploads`. It did run concurrently, so it should not be blocking for too + // long. Otherwise —if `request` has failed— `e2ee_uploads` has + // been dropped, so aborted. + e2ee_uploads.await.map_err(|error| Error::JoinError { + task_description: "e2ee_uploads".to_owned(), + error, + })?; + + response + } else { + request.await? + } + }; - // The code manipulates `Request` and `Response` from Simplified MSC3575 - // because it's simpler. If the `experimental-not-simplified-sliding-sync` - // feature flag is turned on, the Simplified MSC3575 `Request` has been - // transformed into a MSC3575 `Request` a couple of lines above. Now it's - // time to do the opposite: transform the MSC3575 `Response` into a - // Simplified MSC3575 `Response`. - #[cfg(feature = "experimental-not-simplified-sliding-sync")] - let response: http::simplified_msc3575::Response = http::From::from(response); + // Send the request and get a response _without_ end-to-end encryption support. + #[cfg(not(feature = "e2e-encryption"))] + let response = request.await?; - debug!("Received response"); + // The code manipulates `Request` and `Response` from Simplified MSC3575 because + // it's the future standard. But this function may have received a `Request` + // from Simplified MSC3575 or MSC3575. We need to get back a + // Simplified MSC3575 `Response`. + let response = http::Into::::into(response); - // At this point, the request has been sent, and a response has been received. - // - // We must ensure the handling of the response cannot be stopped/ - // cancelled. It must be done entirely, otherwise we can have - // corrupted/incomplete states for Sliding Sync and other parts of - // the code. - // - // That's why we are running the handling of the response in a spawned - // future that cannot be cancelled by anything. - let this = self.clone(); + debug!("Received response"); + + // At this point, the request has been sent, and a response has been received. + // + // We must ensure the handling of the response cannot be stopped/ + // cancelled. It must be done entirely, otherwise we can have + // corrupted/incomplete states for Sliding Sync and other parts of + // the code. + // + // That's why we are running the handling of the response in a spawned + // future that cannot be cancelled by anything. + let this = this.clone(); - // Spawn a new future to ensure that the code inside this future cannot be - // cancelled if this method is cancelled. - let future = async move { - debug!("Start handling response"); + // Spawn a new future to ensure that the code inside this future cannot be + // cancelled if this method is cancelled. + let future = async move { + debug!("Start handling response"); - // In case the task running this future is detached, we must - // ensure responses are handled one at a time. At this point we still own - // `position_guard`, so we're fine. + // In case the task running this future is detached, we must + // ensure responses are handled one at a time. At this point we still own + // `position_guard`, so we're fine. - // Handle the response. - let updates = this.handle_response(response, &mut position_guard).await?; + // Handle the response. + let updates = this.handle_response(response, &mut position_guard).await?; - this.cache_to_storage(&position_guard).await?; + this.cache_to_storage(&position_guard).await?; - // Release the position guard lock. - // It means that other responses can be generated and then handled later. - drop(position_guard); + // Release the position guard lock. + // It means that other responses can be generated and then handled later. + drop(position_guard); - debug!("Done handling response"); + debug!("Done handling response"); - Ok(updates) - }; + Ok(updates) + }; - spawn(future.instrument(Span::current())).await.unwrap() + spawn(future.instrument(Span::current())).await.unwrap() + } } /// Create a _new_ Sliding Sync sync loop. diff --git a/labs/multiverse/Cargo.toml b/labs/multiverse/Cargo.toml index 95027007071..710ddc97236 100644 --- a/labs/multiverse/Cargo.toml +++ b/labs/multiverse/Cargo.toml @@ -14,7 +14,7 @@ color-eyre = "0.6.2" crossterm = "0.27.0" futures-util = { workspace = true } imbl = { workspace = true } -matrix-sdk = { path = "../../crates/matrix-sdk", features = ["sso-login", "experimental-not-simplified-sliding-sync"] } +matrix-sdk = { path = "../../crates/matrix-sdk", features = ["sso-login"] } matrix-sdk-ui = { path = "../../crates/matrix-sdk-ui" } ratatui = "0.26.1" rpassword = "7.3.1" diff --git a/testing/matrix-sdk-integration-testing/Cargo.toml b/testing/matrix-sdk-integration-testing/Cargo.toml index a76d3d1f8b9..8fc88d1ff0a 100644 --- a/testing/matrix-sdk-integration-testing/Cargo.toml +++ b/testing/matrix-sdk-integration-testing/Cargo.toml @@ -17,11 +17,7 @@ futures-core = { workspace = true } futures-util = { workspace = true } http = { workspace = true } matrix-sdk-base = { workspace = true, default-features = true, features = ["testing", "qrcode"] } -matrix-sdk = {workspace = true, default-features = true, features = [ - "testing", - "qrcode", - "experimental-not-simplified-sliding-sync", -] } +matrix-sdk = {workspace = true, default-features = true, features = ["testing", "qrcode"] } matrix-sdk-ui = { workspace = true, default-features = true } matrix-sdk-test = { workspace = true } once_cell = { workspace = true }