Fix leaked signals created in effects

packages/router/src/contexts/router.rs

@@ -2,7 +2,7 @@ use std::{
     any::Any,
     collections::HashSet,
     rc::Rc,
-    sync::{Arc, RwLock, RwLockWriteGuard},
+    sync::{Arc, RwLock},
 };
 
 use dioxus::prelude::*;
@@ -36,7 +36,7 @@ struct MutableRouterState {
 /// A collection of router data that manages all routing functionality.
 #[derive(Clone)]
 pub struct RouterContext {
-    state: Arc<RwLock<MutableRouterState>>,
+    state: Rc<RefCell<MutableRouterState>>,
 
     subscribers: Arc<RwLock<HashSet<ScopeId>>>,
     subscriber_update: Arc<dyn Fn(ScopeId)>,
@@ -56,7 +56,7 @@ impl RouterContext {
         R: Clone,
         <R as std::str::FromStr>::Err: std::fmt::Display,
     {
-        let state = Arc::new(RwLock::new(MutableRouterState {
+        let state = Rc::new(RefCell::new(MutableRouterState {
             prefix: Default::default(),
             history: cfg.take_history(),
             unresolved_error: None,
@@ -105,7 +105,7 @@ impl RouterContext {
 
         // set the updater
         {
-            let mut state = myself.state.write().unwrap();
+            let mut state = myself.state.borrow_mut();
             state.history.updater(Arc::new(move || {
                 for &id in subscribers.read().unwrap().iter() {
                     (mark_dirty)(id);
@@ -117,28 +117,28 @@ impl RouterContext {
     }
 
     pub(crate) fn route_from_str(&self, route: &str) -> Result<Rc<dyn Any>, String> {
-        let state = self.state.read().unwrap();
+        let state = self.state.borrow();
         state.history.parse_route(route)
     }
 
     /// Check whether there is a previous page to navigate back to.
     #[must_use]
     pub fn can_go_back(&self) -> bool {
-        self.state.read().unwrap().history.can_go_back()
+        self.state.borrow().history.can_go_back()
     }
 
     /// Check whether there is a future page to navigate forward to.
     #[must_use]
     pub fn can_go_forward(&self) -> bool {
-        self.state.read().unwrap().history.can_go_forward()
+        self.state.borrow().history.can_go_forward()
     }
 
     /// Go back to the previous location.
     ///
     /// Will fail silently if there is no previous location to go to.
     pub fn go_back(&self) {
         {
-            self.state.write().unwrap().history.go_back();
+            self.state.borrow_mut().history.go_back();
         }
 
         self.change_route();
@@ -149,7 +149,7 @@ impl RouterContext {
     /// Will fail silently if there is no next location to go to.
     pub fn go_forward(&self) {
         {
-            self.state.write().unwrap().history.go_forward();
+            self.state.borrow_mut().history.go_forward();
         }
 
         self.change_route();
@@ -206,8 +206,7 @@ impl RouterContext {
     /// The route that is currently active.
     pub fn current<R: Routable>(&self) -> R {
         self.state
-            .read()
-            .unwrap()
+            .borrow()
             .history
             .current_route()
             .downcast::<R>()
@@ -218,7 +217,7 @@ impl RouterContext {
 
     /// The route that is currently active.
     pub fn current_route_string(&self) -> String {
-        self.any_route_to_string(&*self.state.read().unwrap().history.current_route())
+        self.any_route_to_string(&*self.state.borrow().history.current_route())
     }
 
     pub(crate) fn any_route_to_string(&self, route: &dyn Any) -> String {
@@ -243,7 +242,7 @@ impl RouterContext {
 
     /// The prefix that is currently active.
     pub fn prefix(&self) -> Option<String> {
-        self.state.read().unwrap().prefix.clone()
+        self.state.borrow().prefix.clone()
     }
 
     fn external(&self, external: String) -> Option<ExternalNavigationFailure> {
@@ -261,8 +260,8 @@ impl RouterContext {
         }
     }
 
-    fn state_mut(&self) -> RwLockWriteGuard<MutableRouterState> {
-        self.state.write().unwrap()
+    fn state_mut(&self) -> RefMut<MutableRouterState> {
+        self.state.borrow_mut()
     }
 
     /// Manually subscribe to the current route
@@ -283,15 +282,14 @@ impl RouterContext {
 
     /// Clear any unresolved errors
     pub fn clear_error(&self) {
-        self.state.write().unwrap().unresolved_error = None;
+        self.state.borrow_mut().unresolved_error = None;
 
         self.update_subscribers();
     }
 
     pub(crate) fn render_error<'a>(&self, cx: Scope<'a>) -> Element<'a> {
         self.state
-            .read()
-            .unwrap()
+            .borrow()
             .unresolved_error
             .as_ref()
             .and_then(|_| (self.failure_external_navigation)(cx))

packages/signals/src/effect.rs

@@ -18,7 +18,8 @@ pub(crate) fn get_effect_stack() -> EffectStack {
         Some(rt) => rt,
         None => {
             let store = EffectStack::default();
-            provide_root_context(store).expect("in a virtual dom")
+            provide_root_context(store.clone());
+            store
         }
     }
 }
@@ -54,6 +55,7 @@ pub fn use_effect_with_dependencies<D: Dependency>(
 /// Effects allow you to run code when a signal changes. Effects are run immediately and whenever any signal it reads changes.
 #[derive(Copy, Clone, PartialEq)]
 pub struct Effect {
+    pub(crate) source: ScopeId,
     pub(crate) callback: CopyValue<Box<dyn FnMut()>>,
 }
 
@@ -73,6 +75,7 @@ impl Effect {
     /// The signal will be owned by the current component and will be dropped when the component is dropped.
     pub fn new(callback: impl FnMut() + 'static) -> Self {
         let myself = Self {
+            source: current_scope_id().expect("in a virtual dom"),
             callback: CopyValue::new(Box::new(callback)),
         };
 

packages/signals/src/rt.rs

@@ -2,14 +2,13 @@ use std::cell::{Ref, RefMut};
 
 use std::rc::Rc;
 
-use dioxus_core::prelude::{
-    consume_context, consume_context_from_scope, current_scope_id, provide_context,
-    provide_context_to_scope, provide_root_context,
-};
+use dioxus_core::prelude::*;
 use dioxus_core::ScopeId;
 
 use generational_box::{GenerationalBox, Owner, Store};
 
+use crate::Effect;
+
 fn current_store() -> Store {
     match consume_context() {
         Some(rt) => rt,
@@ -21,12 +20,20 @@ fn current_store() -> Store {
 }
 
 fn current_owner() -> Rc<Owner> {
-    match consume_context() {
-        Some(rt) => rt,
-        None => {
-            let owner = Rc::new(current_store().owner());
-            provide_context(owner).expect("in a virtual dom")
+    match Effect::current() {
+        // If we are inside of an effect, we should use the owner of the effect as the owner of the value.
+        Some(effect) => {
+            let scope_id = effect.source;
+            owner_in_scope(scope_id)
         }
+        // Otherwise either get an owner from the current scope or create a new one.
+        None => match has_context() {
+            Some(rt) => rt,
+            None => {
+                let owner = Rc::new(current_store().owner());
+                provide_context(owner).expect("in a virtual dom")
+            }
+        },
     }
 }
 

packages/signals/src/selector.rs

@@ -74,6 +74,7 @@ pub fn selector<R: PartialEq>(mut f: impl FnMut() -> R + 'static) -> ReadOnlySig
         inner: CopyValue::invalid(),
     };
     let effect = Effect {
+        source: current_scope_id().expect("in a virtual dom"),
         callback: CopyValue::invalid(),
     };
 

packages/web/src/dom.rs

@@ -392,6 +392,7 @@ fn read_input_to_data(target: Element) -> Rc<FormData> {
         .dyn_ref()
         .and_then(|input: &web_sys::HtmlInputElement| {
             input.files().and_then(|files| {
+                #[allow(clippy::arc_with_non_send_sync)]
                 crate::file_engine::WebFileEngine::new(files)
                     .map(|f| std::sync::Arc::new(f) as std::sync::Arc<dyn dioxus_html::FileEngine>)
             })