Skip to content

Commit 75d9ece

Browse files
ealmloffealmloff
authored
Fix stores deadlock (#4950)
* Fix stores deadlock * fix clippy * more clippy fixes * move panic for reading invalid keys into userland instead of in the hashmap iter method * apply the same fixes to btreemap * return a dropped error instead of panicing if the item has been removed
1 parent 82d7802 commit 75d9ece

File tree

5 files changed

+154
-112
lines changed

5 files changed

+154
-112
lines changed

‎Cargo.lock‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/stores/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ description = "Server function macros for Dioxus"
1313
dioxus-core = { workspace = true }
1414
dioxus-signals = { workspace = true }
1515
dioxus-stores-macro = { workspace = true, optional = true }
16+
generational-box.workspace = true
1617

1718
[dev-dependencies]
1819
dioxus = { workspace = true }

‎packages/stores/src/impls/btreemap.rs‎

Lines changed: 49 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,15 @@
11
//! Additional utilities for `BTreeMap` stores.
22
3-
use std::{borrow::Borrow, collections::BTreeMap, hash::Hash, iter::FusedIterator};
3+
use std::{
4+
borrow::Borrow, collections::BTreeMap, hash::Hash, iter::FusedIterator, panic::Location,
5+
};
46

57
use crate::{store::Store, ReadStore};
68
use dioxus_signals::{
79
AnyStorage, BorrowError, BorrowMutError, ReadSignal, Readable, ReadableExt, UnsyncStorage,
810
Writable, WriteLock, WriteSignal,
911
};
12+
use generational_box::ValueDroppedError;
1013

1114
impl<Lens: Readable<Target = BTreeMap<K, V>> + 'static, K: 'static, V: 'static>
1215
Store<BTreeMap<K, V>, Lens>
@@ -77,7 +80,7 @@ impl<Lens: Readable<Target = BTreeMap<K, V>> + 'static, K: 'static, V: 'static>
7780
self.selector().track_shallow();
7881
let keys: Vec<_> = self.selector().peek_unchecked().keys().cloned().collect();
7982
keys.into_iter().map(move |key| {
80-
let value = self.clone().get(key.clone()).unwrap();
83+
let value = self.clone().get_unchecked(key.clone());
8184
(key, value)
8285
})
8386
}
@@ -110,7 +113,7 @@ impl<Lens: Readable<Target = BTreeMap<K, V>> + 'static, K: 'static, V: 'static>
110113
self.selector().track_shallow();
111114
let keys = self.selector().peek().keys().cloned().collect::<Vec<_>>();
112115
keys.into_iter()
113-
.map(move |key| self.clone().get(key).unwrap())
116+
.map(move |key| self.clone().get_unchecked(key))
114117
}
115118

116119
/// Insert a new key-value pair into the BTreeMap. This method will mark the store as shallowly dirty, causing
@@ -256,15 +259,38 @@ impl<Lens: Readable<Target = BTreeMap<K, V>> + 'static, K: 'static, V: 'static>
256259
Q: Hash + Ord + 'static,
257260
K: Borrow<Q> + Ord,
258261
{
259-
self.contains_key(&key).then(|| {
260-
self.into_selector()
261-
.hash_child_unmapped(key.borrow())
262-
.map_writer(move |writer| GetWrite {
263-
index: key,
264-
write: writer,
265-
})
266-
.into()
267-
})
262+
self.contains_key(&key).then(|| self.get_unchecked(key))
263+
}
264+
265+
/// Get a store for the value associated with the given key without checking if the key exists.
266+
/// This method creates a new store scope that tracks just changes to the value associated with the key.
267+
///
268+
/// This is not unsafe, but it will panic when you try to read the value if it does not exist.
269+
///
270+
/// # Example
271+
/// ```rust, no_run
272+
/// use dioxus_stores::*;
273+
/// use dioxus::prelude::*;
274+
/// use std::collections::BTreeMap;
275+
/// let mut store = use_store(|| BTreeMap::new());
276+
/// store.insert(0, "value".to_string());
277+
/// assert_eq!(store.get_unchecked(0).cloned(), "value".to_string());
278+
/// ```
279+
#[track_caller]
280+
pub fn get_unchecked<Q>(self, key: Q) -> Store<V, GetWrite<Q, Lens>>
281+
where
282+
Q: Hash + Ord + 'static,
283+
K: Borrow<Q> + Ord,
284+
{
285+
let created = std::panic::Location::caller();
286+
self.into_selector()
287+
.hash_child_unmapped(key.borrow())
288+
.map_writer(move |writer| GetWrite {
289+
index: key,
290+
write: writer,
291+
created,
292+
})
293+
.into()
268294
}
269295
}
270296

@@ -273,6 +299,7 @@ impl<Lens: Readable<Target = BTreeMap<K, V>> + 'static, K: 'static, V: 'static>
273299
pub struct GetWrite<Index, Write> {
274300
index: Index,
275301
write: Write,
302+
created: &'static Location<'static>,
276303
}
277304

278305
impl<Index, Write, K, V> Readable for GetWrite<Index, Write>
@@ -289,25 +316,19 @@ where
289316
where
290317
Self::Target: 'static,
291318
{
292-
self.write.try_read_unchecked().map(|value| {
293-
Self::Storage::map(value, |value: &Write::Target| {
294-
value
295-
.get(&self.index)
296-
.expect("Tried to access a key that does not exist")
297-
})
319+
self.write.try_read_unchecked().and_then(|value| {
320+
Self::Storage::try_map(value, |value: &Write::Target| value.get(&self.index))
321+
.ok_or_else(|| BorrowError::Dropped(ValueDroppedError::new(self.created)))
298322
})
299323
}
300324

301325
fn try_peek_unchecked(&self) -> Result<dioxus_signals::ReadableRef<'static, Self>, BorrowError>
302326
where
303327
Self::Target: 'static,
304328
{
305-
self.write.try_peek_unchecked().map(|value| {
306-
Self::Storage::map(value, |value: &Write::Target| {
307-
value
308-
.get(&self.index)
309-
.expect("Tried to access a key that does not exist")
310-
})
329+
self.write.try_peek_unchecked().and_then(|value| {
330+
Self::Storage::try_map(value, |value: &Write::Target| value.get(&self.index))
331+
.ok_or_else(|| BorrowError::Dropped(ValueDroppedError::new(self.created)))
311332
})
312333
}
313334

@@ -333,12 +354,11 @@ where
333354
where
334355
Self::Target: 'static,
335356
{
336-
self.write.try_write_unchecked().map(|value| {
337-
WriteLock::map(value, |value: &mut Write::Target| {
338-
value
339-
.get_mut(&self.index)
340-
.expect("Tried to access a key that does not exist")
357+
self.write.try_write_unchecked().and_then(|value| {
358+
WriteLock::filter_map(value, |value: &mut Write::Target| {
359+
value.get_mut(&self.index)
341360
})
361+
.ok_or_else(|| BorrowMutError::Dropped(ValueDroppedError::new(self.created)))
342362
})
343363
}
344364
}

‎packages/stores/src/impls/hashmap.rs‎

Lines changed: 49 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,15 @@ use std::{
55
collections::HashMap,
66
hash::{BuildHasher, Hash},
77
iter::FusedIterator,
8+
panic::Location,
89
};
910

1011
use crate::{store::Store, ReadStore};
1112
use dioxus_signals::{
1213
AnyStorage, BorrowError, BorrowMutError, ReadSignal, Readable, ReadableExt, UnsyncStorage,
1314
Writable, WriteLock, WriteSignal,
1415
};
16+
use generational_box::ValueDroppedError;
1517

1618
impl<Lens: Readable<Target = HashMap<K, V, St>> + 'static, K: 'static, V: 'static, St: 'static>
1719
Store<HashMap<K, V, St>, Lens>
@@ -82,10 +84,8 @@ impl<Lens: Readable<Target = HashMap<K, V, St>> + 'static, K: 'static, V: 'stati
8284
{
8385
self.selector().track_shallow();
8486
let keys: Vec<_> = self.selector().peek_unchecked().keys().cloned().collect();
85-
keys.into_iter().map(move |key| {
86-
let value = self.clone().get(key.clone()).unwrap();
87-
(key, value)
88-
})
87+
keys.into_iter()
88+
.map(move |key| (key.clone(), self.clone().get_unchecked(key)))
8989
}
9090

9191
/// Get an iterator over the values in the HashMap. This method will track the store shallowly and only cause
@@ -117,7 +117,7 @@ impl<Lens: Readable<Target = HashMap<K, V, St>> + 'static, K: 'static, V: 'stati
117117
self.selector().track_shallow();
118118
let keys = self.selector().peek().keys().cloned().collect::<Vec<_>>();
119119
keys.into_iter()
120-
.map(move |key| self.clone().get(key).unwrap())
120+
.map(move |key| self.clone().get_unchecked(key))
121121
}
122122

123123
/// Insert a new key-value pair into the HashMap. This method will mark the store as shallowly dirty, causing
@@ -264,15 +264,39 @@ impl<Lens: Readable<Target = HashMap<K, V, St>> + 'static, K: 'static, V: 'stati
264264
K: Borrow<Q> + Eq + Hash,
265265
St: BuildHasher,
266266
{
267-
self.contains_key(&key).then(|| {
268-
self.into_selector()
269-
.hash_child_unmapped(key.borrow())
270-
.map_writer(move |writer| GetWrite {
271-
index: key,
272-
write: writer,
273-
})
274-
.into()
275-
})
267+
self.contains_key(&key).then(|| self.get_unchecked(key))
268+
}
269+
270+
/// Get a store for the value associated with the given key without checking if the key exists.
271+
/// This method creates a new store scope that tracks just changes to the value associated with the key.
272+
///
273+
/// This is not unsafe, but it will panic when you try to read the value if it does not exist.
274+
///
275+
/// # Example
276+
/// ```rust, no_run
277+
/// use dioxus_stores::*;
278+
/// use dioxus::prelude::*;
279+
/// use std::collections::HashMap;
280+
/// let mut store = use_store(|| HashMap::new());
281+
/// store.insert(0, "value".to_string());
282+
/// assert_eq!(store.get_unchecked(0).cloned(), "value".to_string());
283+
/// ```
284+
#[track_caller]
285+
pub fn get_unchecked<Q>(self, key: Q) -> Store<V, GetWrite<Q, Lens>>
286+
where
287+
Q: Hash + Eq + 'static,
288+
K: Borrow<Q> + Eq + Hash,
289+
St: BuildHasher,
290+
{
291+
let location = Location::caller();
292+
self.into_selector()
293+
.hash_child_unmapped(key.borrow())
294+
.map_writer(move |writer| GetWrite {
295+
index: key,
296+
write: writer,
297+
created: location,
298+
})
299+
.into()
276300
}
277301
}
278302

@@ -281,6 +305,7 @@ impl<Lens: Readable<Target = HashMap<K, V, St>> + 'static, K: 'static, V: 'stati
281305
pub struct GetWrite<Index, Write> {
282306
index: Index,
283307
write: Write,
308+
created: &'static Location<'static>,
284309
}
285310

286311
impl<Index, Write, K, V, St> Readable for GetWrite<Index, Write>
@@ -298,25 +323,19 @@ where
298323
where
299324
Self::Target: 'static,
300325
{
301-
self.write.try_read_unchecked().map(|value| {
302-
Self::Storage::map(value, |value: &Write::Target| {
303-
value
304-
.get(&self.index)
305-
.expect("Tried to access a key that does not exist")
306-
})
326+
self.write.try_read_unchecked().and_then(|value| {
327+
Self::Storage::try_map(value, |value: &Write::Target| value.get(&self.index))
328+
.ok_or_else(|| BorrowError::Dropped(ValueDroppedError::new(self.created)))
307329
})
308330
}
309331

310332
fn try_peek_unchecked(&self) -> Result<dioxus_signals::ReadableRef<'static, Self>, BorrowError>
311333
where
312334
Self::Target: 'static,
313335
{
314-
self.write.try_peek_unchecked().map(|value| {
315-
Self::Storage::map(value, |value: &Write::Target| {
316-
value
317-
.get(&self.index)
318-
.expect("Tried to access a key that does not exist")
319-
})
336+
self.write.try_peek_unchecked().and_then(|value| {
337+
Self::Storage::try_map(value, |value: &Write::Target| value.get(&self.index))
338+
.ok_or_else(|| BorrowError::Dropped(ValueDroppedError::new(self.created)))
320339
})
321340
}
322341

@@ -343,12 +362,11 @@ where
343362
where
344363
Self::Target: 'static,
345364
{
346-
self.write.try_write_unchecked().map(|value| {
347-
WriteLock::map(value, |value: &mut Write::Target| {
348-
value
349-
.get_mut(&self.index)
350-
.expect("Tried to access a key that does not exist")
365+
self.write.try_write_unchecked().and_then(|value| {
366+
WriteLock::filter_map(value, |value: &mut Write::Target| {
367+
value.get_mut(&self.index)
351368
})
369+
.ok_or_else(|| BorrowMutError::Dropped(ValueDroppedError::new(self.created)))
352370
})
353371
}
354372
}

0 commit comments

Comments
 (0)