Skip to content

Commit bfad283

Browse files
committed
fix(base): Move all fields of MemoryStore inside a StdRwLock<_>.
This patch creates a new `MemoryStoreInner` and moves all fields from `MemoryStore` into this new type. All locks are removed, but a new lock is added around `MemoryStoreInner`. That way we have a single lock.
1 parent ba73489 commit bfad283

File tree

3 files changed

+42
-25
lines changed

3 files changed

+42
-25
lines changed

crates/matrix-sdk-base/src/event_cache/store/memory_store.rs

Lines changed: 38 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,14 @@ use crate::{
3434
#[allow(clippy::type_complexity)]
3535
#[derive(Debug)]
3636
pub struct MemoryStore {
37-
media: StdRwLock<RingBuffer<(OwnedMxcUri, String /* unique key */, Vec<u8>)>>,
38-
leases: StdRwLock<HashMap<String, (String, Instant)>>,
39-
events: StdRwLock<RelationalLinkedChunk<Event, Gap>>,
37+
inner: StdRwLock<MemoryStoreInner>,
38+
}
39+
40+
#[derive(Debug)]
41+
struct MemoryStoreInner {
42+
media: RingBuffer<(OwnedMxcUri, String /* unique key */, Vec<u8>)>,
43+
leases: HashMap<String, (String, Instant)>,
44+
events: RelationalLinkedChunk<Event, Gap>,
4045
}
4146

4247
// SAFETY: `new_unchecked` is safe because 20 is not zero.
@@ -45,9 +50,11 @@ const NUMBER_OF_MEDIAS: NonZeroUsize = unsafe { NonZeroUsize::new_unchecked(20)
4550
impl Default for MemoryStore {
4651
fn default() -> Self {
4752
Self {
48-
media: StdRwLock::new(RingBuffer::new(NUMBER_OF_MEDIAS)),
49-
leases: Default::default(),
50-
events: StdRwLock::new(RelationalLinkedChunk::new()),
53+
inner: StdRwLock::new(MemoryStoreInner {
54+
media: RingBuffer::new(NUMBER_OF_MEDIAS),
55+
leases: Default::default(),
56+
events: RelationalLinkedChunk::new(),
57+
}),
5158
}
5259
}
5360
}
@@ -70,17 +77,19 @@ impl EventCacheStore for MemoryStore {
7077
key: &str,
7178
holder: &str,
7279
) -> Result<bool, Self::Error> {
73-
Ok(try_take_leased_lock(&self.leases, lease_duration_ms, key, holder))
80+
let mut inner = self.inner.write().unwrap();
81+
82+
Ok(try_take_leased_lock(&mut inner.leases, lease_duration_ms, key, holder))
7483
}
7584

7685
async fn handle_linked_chunk_updates(
7786
&self,
7887
room_id: &RoomId,
7988
updates: &[Update<Event, Gap>],
8089
) -> Result<(), Self::Error> {
81-
self.events.write().unwrap().apply_updates(room_id, updates);
90+
let mut inner = self.inner.write().unwrap();
8291

83-
Ok(())
92+
Ok(inner.events.apply_updates(updates))
8493
}
8594

8695
async fn add_media_content(
@@ -90,8 +99,10 @@ impl EventCacheStore for MemoryStore {
9099
) -> Result<()> {
91100
// Avoid duplication. Let's try to remove it first.
92101
self.remove_media_content(request).await?;
102+
93103
// Now, let's add it.
94-
self.media.write().unwrap().push((request.uri().to_owned(), request.unique_key(), data));
104+
let mut inner = self.inner.write().unwrap();
105+
inner.media.push((request.uri().to_owned(), request.unique_key(), data));
95106

96107
Ok(())
97108
}
@@ -103,8 +114,10 @@ impl EventCacheStore for MemoryStore {
103114
) -> Result<(), Self::Error> {
104115
let expected_key = from.unique_key();
105116

106-
let mut medias = self.media.write().unwrap();
107-
if let Some((mxc, key, _)) = medias.iter_mut().find(|(_, key, _)| *key == expected_key) {
117+
let mut inner = self.inner.write().unwrap();
118+
119+
if let Some((mxc, key, _)) = inner.media.iter_mut().find(|(_, key, _)| *key == expected_key)
120+
{
108121
*mxc = to.uri().to_owned();
109122
*key = to.unique_key();
110123
}
@@ -115,32 +128,37 @@ impl EventCacheStore for MemoryStore {
115128
async fn get_media_content(&self, request: &MediaRequestParameters) -> Result<Option<Vec<u8>>> {
116129
let expected_key = request.unique_key();
117130

118-
let media = self.media.read().unwrap();
119-
Ok(media.iter().find_map(|(_media_uri, media_key, media_content)| {
131+
let inner = self.inner.write().unwrap();
132+
133+
Ok(inner.media.iter().find_map(|(_media_uri, media_key, media_content)| {
120134
(media_key == &expected_key).then(|| media_content.to_owned())
121135
}))
122136
}
123137

124138
async fn remove_media_content(&self, request: &MediaRequestParameters) -> Result<()> {
125139
let expected_key = request.unique_key();
126140

127-
let mut media = self.media.write().unwrap();
128-
let Some(index) = media
141+
let mut inner = self.inner.write().unwrap();
142+
143+
let Some(index) = inner
144+
.media
129145
.iter()
130146
.position(|(_media_uri, media_key, _media_content)| media_key == &expected_key)
131147
else {
132148
return Ok(());
133149
};
134150

135-
media.remove(index);
151+
inner.media.remove(index);
136152

137153
Ok(())
138154
}
139155

140156
async fn remove_media_content_for_uri(&self, uri: &MxcUri) -> Result<()> {
141-
let mut media = self.media.write().unwrap();
157+
let mut inner = self.inner.write().unwrap();
158+
142159
let expected_key = uri.to_owned();
143-
let positions = media
160+
let positions = inner
161+
.media
144162
.iter()
145163
.enumerate()
146164
.filter_map(|(position, (media_uri, _media_key, _media_content))| {
@@ -150,7 +168,7 @@ impl EventCacheStore for MemoryStore {
150168

151169
// Iterate in reverse-order so that positions stay valid after first removals.
152170
for position in positions.into_iter().rev() {
153-
media.remove(position);
171+
inner.media.remove(position);
154172
}
155173

156174
Ok(())

crates/matrix-sdk-common/src/store_locks.rs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -361,7 +361,7 @@ mod tests {
361361

362362
impl TestStore {
363363
fn try_take_leased_lock(&self, lease_duration_ms: u32, key: &str, holder: &str) -> bool {
364-
try_take_leased_lock(&self.leases, lease_duration_ms, key, holder)
364+
try_take_leased_lock(&mut self.leases.write().unwrap(), lease_duration_ms, key, holder)
365365
}
366366
}
367367

@@ -502,20 +502,19 @@ mod tests {
502502
pub mod memory_store_helper {
503503
use std::{
504504
collections::{hash_map::Entry, HashMap},
505-
sync::RwLock,
506505
time::{Duration, Instant},
507506
};
508507

509508
pub fn try_take_leased_lock(
510-
leases: &RwLock<HashMap<String, (String, Instant)>>,
509+
leases: &mut HashMap<String, (String, Instant)>,
511510
lease_duration_ms: u32,
512511
key: &str,
513512
holder: &str,
514513
) -> bool {
515514
let now = Instant::now();
516515
let expiration = now + Duration::from_millis(lease_duration_ms.into());
517516

518-
match leases.write().unwrap().entry(key.to_owned()) {
517+
match leases.entry(key.to_owned()) {
519518
// There is an existing holder.
520519
Entry::Occupied(mut entry) => {
521520
let (current_holder, current_expiration) = entry.get_mut();

crates/matrix-sdk-crypto/src/store/memorystore.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -632,7 +632,7 @@ impl CryptoStore for MemoryStore {
632632
key: &str,
633633
holder: &str,
634634
) -> Result<bool> {
635-
Ok(try_take_leased_lock(&self.leases, lease_duration_ms, key, holder))
635+
Ok(try_take_leased_lock(&mut self.leases.write().unwrap(), lease_duration_ms, key, holder))
636636
}
637637
}
638638

0 commit comments

Comments
 (0)