From 115bf6c9de145ca44c75b9630faa186d4f7be118 Mon Sep 17 00:00:00 2001
From: Kegan Dougal <7190048+kegsay@users.noreply.github.com>
Date: Tue, 9 Jul 2024 09:44:51 +0100
Subject: [PATCH] memory-store: release locks earlier to avoid deadlocks

I've been debugging a cause of flakey complement-crypto tests for
about a month now. I was pretty convinced it was deadlocking
somewhere in the memory store `save_changes` code. With additional
logging, it's now clear that the there is an ABBA style deadlock
when `save_changes` is called at the same time as `get_state_events`.

I've also adjusted code for `get_user_ids` as it has a very similar
pattern and also acquires locks in reverse order to `save_changes`,
so is potentially vulnerable to this.
---
 .../matrix-sdk-base/src/store/memory_store.rs | 33 +++++++++----------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/crates/matrix-sdk-base/src/store/memory_store.rs b/crates/matrix-sdk-base/src/store/memory_store.rs
index 5db4db8133..85d276f7b4 100644
--- a/crates/matrix-sdk-base/src/store/memory_store.rs
+++ b/crates/matrix-sdk-base/src/store/memory_store.rs
@@ -579,21 +579,18 @@ impl StateStore for MemoryStore {
             Some(state_events.values().cloned().map(to_enum).collect())
         }
 
-        Ok(get_events(
-            &self.stripped_room_state.read().unwrap(),
-            room_id,
-            &event_type,
-            RawAnySyncOrStrippedState::Stripped,
-        )
-        .or_else(|| {
-            get_events(
-                &self.room_state.read().unwrap(),
-                room_id,
-                &event_type,
-                RawAnySyncOrStrippedState::Sync,
-            )
-        })
-        .unwrap_or_default())
+        let state_map = self.stripped_room_state.read().unwrap();
+        Ok(get_events(&state_map, room_id, &event_type, RawAnySyncOrStrippedState::Stripped)
+            .or_else(|| {
+                drop(state_map); // release the lock on stripped_room_state
+                get_events(
+                    &self.room_state.read().unwrap(),
+                    room_id,
+                    &event_type,
+                    RawAnySyncOrStrippedState::Sync,
+                )
+            })
+            .unwrap_or_default())
     }
 
     async fn get_state_events_for_keys(
@@ -702,12 +699,12 @@ impl StateStore for MemoryStore {
                 })
                 .unwrap_or_default()
         }
-        trace!("getting stripped_members lock");
-        let v = get_user_ids_inner(&self.stripped_members.read().unwrap(), room_id, memberships);
+        let state_map = self.stripped_members.read().unwrap();
+        let v = get_user_ids_inner(&state_map, room_id, memberships);
         if !v.is_empty() {
             return Ok(v);
         }
-        trace!("getting members lock");
+        drop(state_map); // release the stripped_members lock
         Ok(get_user_ids_inner(&self.members.read().unwrap(), room_id, memberships))
     }