Fix subscriptions for read only signals passed as props (#3173)

* add a fuzzing test for reference counted generational boxes

* Fix subscriptions for readonlysignal passed as props

* Don't subscribe to dropped contexts

---------

Co-authored-by: Jonathan Kelley <jkelleyrtp@gmail.com>

packages/core/src/reactive_context.rs

@@ -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,
@@ -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);
+        match self.inner.try_write() {
-        self.inner
+            Ok(mut inner) => {
-            .write()
+                subscriptions.lock().unwrap().insert(*self);
-            .subscribers
+                inner.subscribers.insert(PointerHash(subscriptions));
-            .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

packages/generational-box/src/lib.rs

@@ -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)
     }
 }

packages/generational-box/tests/basic.rs

@@ -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);
@@ -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);
@@ -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());
+    }
+}

packages/generational-box/tests/reference_counting.rs

@@ -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);
 

packages/signals/src/copy_value.rs

@@ -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)
     }
 

packages/signals/src/read_only_signal.rs

@@ -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)
     }
 

packages/signals/src/signal.rs

@@ -221,8 +221,14 @@ impl<T: 'static, S: Storage<SignalData<T>>> Signal<T, S> {
         }
     }
 
-    /// Point to another signal
+    /// Point to another signal. This will subscribe the other signal to all subscribers of this signal.
-    pub fn point_to(&mut self, other: Self) -> BorrowResult {
+    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)
     }