feat(sdk): RoomEventCacheInner::backpaginate always return Ok for unknown token.

Prior to this patch, in `RoomEventCacheInner::backpaginate`, when the
`token` validity was checked, and it was invalid:

* before calling `/messages`, `Err(EventCacheError::UnknownBackpaginationToken)` was returned,
* after calling `/messages`, `Ok(BackPaginationOutput::UnknownBackpaginationToken)` was returned.

This patch tries to uniformize this by only returning
`Ok(BackPaginationOutput::UnknownBackpaginationToken)`.

That's a tradeoff. It will probably be refactor later.

The idea is also to call `/messages` **before** taking the write-lock
of `RoomEvents`, otherwise it can keep the lock for up to 30secs in
this case. Also, checking the validity of the `token` **before** and
**after** `/messages` is not necessary: it can be done only after.

Author: Hywan

crates/matrix-sdk/src/event_cache/mod.rs

@@ -632,9 +632,18 @@ impl RoomEventCacheInner {
         // Make sure there's at most one back-pagination request.
         let _guard = self.pagination_lock.lock().await;
 
-        // Make sure the `RoomEvents` isn't updated while we are back-paginating. This
-        // is really important, for example if a sync is happening while we are
-        // back-paginating.
+        // Get messages.
+        let messages = self
+            .room
+            .messages(assign!(MessagesOptions::backward(), {
+                from: token.as_ref().map(|token| token.0.clone()),
+                limit: batch_size.into()
+            }))
+            .await
+            .map_err(EventCacheError::SdkError)?;
+
+        // Make sure the `RoomEvents` isn't updated while we are saving events from
+        // backpagination.
         let mut room_events = self.events.write().await;
 
         // Check that the `token` exists if any.
@@ -646,24 +655,14 @@ impl RoomEventCacheInner {
             // The method has been called with `token` but it doesn't exist in `RoomEvents`,
             // it's an error.
             if gap_identifier.is_none() {
-                return Err(EventCacheError::UnknownBackpaginationToken);
+                return Ok(BackPaginationOutcome::UnknownBackpaginationToken);
             }
 
             gap_identifier
         } else {
             None
         };
 
-        // Get messages.
-        let messages = self
-            .room
-            .messages(assign!(MessagesOptions::backward(), {
-                from: token.as_ref().map(|token| token.0.clone()),
-                limit: batch_size.into()
-            }))
-            .await
-            .map_err(EventCacheError::SdkError)?;
-
         // Would we want to backpaginate again, we'd start from the `end` token as the
         // next `from` token.
 
@@ -834,7 +833,7 @@ mod tests {
     use matrix_sdk_test::{async_test, sync_timeline_event};
     use ruma::room_id;
 
-    use super::EventCacheError;
+    use super::{BackPaginationOutcome, EventCacheError};
     use crate::{event_cache::store::PaginationToken, test_utils::logged_in_client};
 
     #[async_test]
@@ -853,35 +852,57 @@ mod tests {
         assert_matches!(result, Err(EventCacheError::NotSubscribedYet));
     }
 
-    #[async_test]
-    async fn test_unknown_pagination_token() {
-        let client = logged_in_client(None).await;
-        let room_id = room_id!("!galette:saucisse.bzh");
-        client.base_client().get_or_create_room(room_id, matrix_sdk_base::RoomState::Joined);
-
-        client.event_cache().subscribe().unwrap();
-
-        let (room_event_cache, _drop_handles) =
-            client.event_cache().for_room(room_id).await.unwrap();
-        let room_event_cache = room_event_cache.unwrap();
-
-        // If I try to back-paginate with an unknown back-pagination token,
-        let token = PaginationToken("old".to_owned());
-
-        // Then I run into an error.
-        let res = room_event_cache.backpaginate(20, Some(token)).await;
-        assert_matches!(res.unwrap_err(), EventCacheError::UnknownBackpaginationToken);
-    }
-
     // Those tests require time to work, and it does not on wasm32.
     #[cfg(not(target_arch = "wasm32"))]
     mod time_tests {
         use std::time::{Duration, Instant};
 
         use matrix_sdk_base::RoomState;
+        use serde_json::json;
         use tokio::time::sleep;
+        use wiremock::{
+            matchers::{header, method, path_regex, query_param},
+            Mock, ResponseTemplate,
+        };
 
         use super::{super::store::Gap, *};
+        use crate::test_utils::logged_in_client_with_server;
+
+        #[async_test]
+        async fn test_unknown_pagination_token() {
+            let (client, server) = logged_in_client_with_server().await;
+
+            let room_id = room_id!("!galette:saucisse.bzh");
+            client.base_client().get_or_create_room(room_id, matrix_sdk_base::RoomState::Joined);
+
+            client.event_cache().subscribe().unwrap();
+
+            let (room_event_cache, _drop_handles) =
+                client.event_cache().for_room(room_id).await.unwrap();
+            let room_event_cache = room_event_cache.unwrap();
+
+            // If I try to back-paginate with an unknown back-pagination token,
+            let token_name = "unknown";
+            let token = PaginationToken(token_name.to_owned());
+
+            // Then I run into an error.
+            Mock::given(method("GET"))
+                .and(path_regex(r"^/_matrix/client/r0/rooms/.*/messages$"))
+                .and(header("authorization", "Bearer 1234"))
+                .and(query_param("from", token_name))
+                .respond_with(ResponseTemplate::new(200).set_body_json(json!({
+                    "start": token_name,
+                    "chunk": [],
+                })))
+                .expect(1)
+                .mount(&server)
+                .await;
+
+            let res = room_event_cache.backpaginate(20, Some(token)).await;
+            assert_matches!(res, Ok(BackPaginationOutcome::UnknownBackpaginationToken));
+
+            server.verify().await
+        }
 
         #[async_test]
         async fn test_wait_no_pagination_token() {

crates/matrix-sdk/tests/integration/event_cache.rs

@@ -515,17 +515,11 @@ async fn test_reset_while_backpaginating() {
     mock_sync(&server, sync_response_body, None).await;
     client.sync_once(Default::default()).await.unwrap();
 
-    let outcome = backpagination.await.expect("join failed").unwrap();
+    let outcome = backpagination.await.expect("join failed");
 
-    // Backpagination should have been executed before the sync, despite the
-    // concurrency here. The backpagination should have acquired a write lock before
-    // the sync.
-    {
-        assert_let!(BackPaginationOutcome::Success { events, reached_start } = outcome);
-        assert!(!reached_start);
-        assert_event_matches_msg(&events[0].clone().into(), "lalala");
-        assert_eq!(events.len(), 1);
-    }
+    // Backpagination should be confused, and the operation should result in an
+    // unknown token.
+    assert_matches!(outcome, Ok(BackPaginationOutcome::UnknownBackpaginationToken));
 
     // Now if we retrieve the earliest token, it's not the one we had before.
     // Even better, it's the one from the sync, NOT from the backpagination!