Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix subscriptions for read only signals passed as props #3173

Merged
merged 9 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
591 changes: 413 additions & 178 deletions Cargo.lock

Large diffs are not rendered by default.

20 changes: 14 additions & 6 deletions packages/core/src/reactive_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
Runtime,
};
use futures_channel::mpsc::UnboundedReceiver;
use generational_box::{GenerationalBox, SyncStorage};
use generational_box::{BorrowMutError, GenerationalBox, SyncStorage};
use std::{
cell::RefCell,
collections::HashSet,
Expand Down Expand Up @@ -243,11 +243,19 @@ impl ReactiveContext {

/// Subscribe to this context. The reactive context will automatically remove itself from the subscriptions when it is reset.
pub fn subscribe(&self, subscriptions: Arc<Mutex<HashSet<ReactiveContext>>>) {
subscriptions.lock().unwrap().insert(*self);
self.inner
.write()
.subscribers
.insert(PointerHash(subscriptions));
match self.inner.try_write() {
Ok(mut inner) => {
subscriptions.lock().unwrap().insert(*self);
inner.subscribers.insert(PointerHash(subscriptions));
}
// If the context was dropped, we don't need to subscribe to it anymore
Err(BorrowMutError::Dropped(_)) => {}
Err(expect) => {
panic!(
"Expected to be able to write to reactive context to subscribe, but it failed with: {expect:?}"
);
}
}
}

/// Get the scope that inner CopyValue is associated with
Expand Down
2 changes: 1 addition & 1 deletion packages/generational-box/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ impl<T, S: Storage<T>> GenerationalBox<T, S> {
}

/// Change this box to point to another generational box
pub fn point_to(&mut self, other: GenerationalBox<T, S>) -> BorrowResult {
pub fn point_to(&self, other: GenerationalBox<T, S>) -> BorrowResult {
S::change_reference(self.raw, other.raw)
}
}
Expand Down
81 changes: 80 additions & 1 deletion packages/generational-box/tests/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ fn fuzz() {

for i in 0..children {
let owner = S::owner();
let key = owner.insert(format!("hello world {path:?}"));
let value = format!("hello world {path:?}");
let key = owner.insert(value.clone());
println!("created new box {key:?}");
valid_keys.push(key);
path.push(i);
Expand All @@ -134,6 +135,12 @@ fn fuzz() {
assert!(key.try_read().is_err());
}
maybe_owner_scope(valid_keys, invalid_keys, path);

// After all the children run, we should still have our data
let key_value = &*key.read();
println!("{:?}", key_value);
assert_eq!(key_value, &value);

let invalid = valid_keys.pop().unwrap();
println!("popping {invalid:?}");
invalid_keys.push(invalid);
Expand All @@ -149,3 +156,75 @@ fn fuzz() {
maybe_owner_scope::<SyncStorage>(&mut Vec::new(), &mut Vec::new(), &mut Vec::new());
}
}

#[test]
fn fuzz_rc() {
fn maybe_owner_scope<S: Storage<String>>(
valid_keys: &mut Vec<Vec<GenerationalBox<String, S>>>,
invalid_keys: &mut Vec<GenerationalBox<String, S>>,
path: &mut Vec<u8>,
) {
let branch_cutoff = 5;
let children = if path.len() < branch_cutoff {
rand::random::<u8>() % 4
} else {
rand::random::<u8>() % 2
};

for i in 0..children {
let owner = S::owner();
let value = format!("hello world {path:?}");
let key = owner.insert_rc(value.clone());
println!("created new box {key:?}");
let mut keys = vec![key];
for _ in 0..rand::random::<u8>() % 10 {
if rand::random::<u8>() % 2 == 0 {
let owner = S::owner();
let key = owner.insert_reference(key).unwrap();
println!("created new reference {key:?}");
invalid_keys.push(key);
}
let key = owner.insert_reference(key).unwrap();
println!("created new reference {key:?}");
keys.push(key);
}
valid_keys.push(keys.clone());
path.push(i);
// read all keys
println!("{:?}", path);
for keys in valid_keys.iter() {
for key in keys {
println!("reading {key:?}");
let value = key.read();
println!("{:?}", &*value);
assert!(value.starts_with("hello world"));
}
}
for key in invalid_keys.iter() {
println!("reading invalid {key:?}");
assert!(key.try_read().is_err());
}
maybe_owner_scope(valid_keys, invalid_keys, path);

// After all the children run, we should still have our data
for key in keys {
let key_value = &*key.read();
println!("{:?}", key_value);
assert_eq!(key_value, &value);
}

let invalid = valid_keys.pop().unwrap();
println!("popping {invalid:?}");
invalid_keys.extend(invalid);
path.pop();
}
}

for _ in 0..10 {
maybe_owner_scope::<UnsyncStorage>(&mut Vec::new(), &mut Vec::new(), &mut Vec::new());
}

for _ in 0..10 {
maybe_owner_scope::<SyncStorage>(&mut Vec::new(), &mut Vec::new(), &mut Vec::new());
}
}
2 changes: 1 addition & 1 deletion packages/generational-box/tests/reference_counting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ fn move_reference_in_place() {
let original_owner = S::owner();
// insert data into the store
let original = original_owner.insert_rc(data1.clone());
let mut reference = original_owner.insert_reference(original).unwrap();
let reference = original_owner.insert_reference(original).unwrap();
// The reference should point to the original value
assert_eq!(&*reference.read(), &data1);

Expand Down
2 changes: 1 addition & 1 deletion packages/signals/src/copy_value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl<T: 'static, S: Storage<T>> CopyValue<T, S> {
}

/// Point to another copy value
pub fn point_to(&mut self, other: Self) -> BorrowResult {
pub fn point_to(&self, other: Self) -> BorrowResult {
self.value.point_to(other.value)
}

Expand Down
2 changes: 1 addition & 1 deletion packages/signals/src/read_only_signal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ impl<T: 'static, S: Storage<SignalData<T>>> ReadOnlySignal<T, S> {
}

/// Point to another signal
pub fn point_to(&mut self, other: Self) -> BorrowResult {
pub fn point_to(&self, other: Self) -> BorrowResult {
self.inner.point_to(other.inner)
}

Expand Down
10 changes: 8 additions & 2 deletions packages/signals/src/signal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,14 @@ impl<T: 'static, S: Storage<SignalData<T>>> Signal<T, S> {
}
}

/// Point to another signal
pub fn point_to(&mut self, other: Self) -> BorrowResult {
/// Point to another signal. This will subscribe the other signal to all subscribers of this signal.
pub fn point_to(&self, other: Self) -> BorrowResult {
#[allow(clippy::mutable_key_type)]
let this_subscribers = self.inner.value.read().subscribers.lock().unwrap().clone();
let other_read = other.inner.value.read();
for subscriber in this_subscribers.iter() {
subscriber.subscribe(other_read.subscribers.clone());
}
self.inner.point_to(other.inner)
}

Expand Down
Loading