Fix effects triggered from async tasks; improve work scheduling docs (#2370)

* outline work scheduling

* move scheduling code from dirty_scope to scheduler

* introduce queue_effect

* Run effects after all tasks are polled if there are no components rerendered

* add a new test for async effects

packages/core/src/effect.rs

@@ -0,0 +1,66 @@
+use crate::innerlude::ScopeOrder;
+use crate::Runtime;
+use std::borrow::Borrow;
+use std::cell::RefCell;
+use std::collections::VecDeque;
+
+/// Effects will always run after all changes to the DOM have been applied.
+///
+/// Effects are the lowest priority task in the scheduler.
+/// They are run after all other dirty scopes and futures have been resolved. Other dirty scopes and futures may cause the component this effect is attached to to rerun, which would update the DOM.
+pub(crate) struct Effect {
+    // The scope that the effect is attached to
+    pub(crate) order: ScopeOrder,
+    // The callbacks that will be run when effects are rerun
+    effect: RefCell<VecDeque<Box<dyn FnOnce() + 'static>>>,
+}
+
+impl Effect {
+    pub(crate) fn new(order: ScopeOrder, f: impl FnOnce() + 'static) -> Self {
+        let mut effect = VecDeque::new();
+        effect.push_back(Box::new(f) as Box<dyn FnOnce() + 'static>);
+        Self {
+            order,
+            effect: RefCell::new(effect),
+        }
+    }
+
+    pub(crate) fn push_back(&self, f: impl FnOnce() + 'static) {
+        self.effect.borrow_mut().push_back(Box::new(f));
+    }
+
+    pub(crate) fn run(&self, runtime: &Runtime) {
+        runtime.rendering.set(false);
+        let mut effect = self.effect.borrow_mut();
+        while let Some(f) = effect.pop_front() {
+            f();
+        }
+        runtime.rendering.set(true);
+    }
+}
+
+impl PartialOrd for Effect {
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        Some(self.order.cmp(&other.order))
+    }
+}
+
+impl Ord for Effect {
+    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
+        self.order.cmp(&other.order)
+    }
+}
+
+impl PartialEq for Effect {
+    fn eq(&self, other: &Self) -> bool {
+        self.order == other.order
+    }
+}
+
+impl Eq for Effect {}
+
+impl Borrow<ScopeOrder> for Effect {
+    fn borrow(&self) -> &ScopeOrder {
+        &self.order
+    }
+}

packages/core/src/global_context.rs

@@ -90,6 +90,11 @@ pub fn spawn(fut: impl Future<Output = ()> + 'static) -> Task {
     Runtime::with_current_scope(|cx| cx.spawn(fut)).expect("to be in a dioxus runtime")
 }
 
+/// Queue an effect to run after the next render
+pub fn queue_effect(f: impl FnOnce() + 'static) {
+    Runtime::with_current_scope(|cx| cx.queue_effect(f)).expect("to be in a dioxus runtime")
+}
+
 /// Spawn a future that Dioxus won't clean up when this component is unmounted
 ///
 /// This is good for tasks that need to be run after the component has been dropped.

packages/core/src/lib.rs

@@ -6,7 +6,7 @@
 mod any_props;
 mod arena;
 mod diff;
-mod dirty_scope;
+mod effect;
 mod error_boundary;
 mod events;
 mod fragment;
@@ -17,6 +17,7 @@ mod nodes;
 mod properties;
 mod render_signal;
 mod runtime;
+mod scheduler;
 mod scope_arena;
 mod scope_context;
 mod scopes;
@@ -26,7 +27,7 @@ mod virtual_dom;
 pub(crate) mod innerlude {
     pub(crate) use crate::any_props::*;
     pub use crate::arena::*;
-    pub use crate::dirty_scope::*;
+    pub(crate) use crate::effect::*;
     pub use crate::error_boundary::*;
     pub use crate::events::*;
     pub use crate::fragment::*;
@@ -36,6 +37,7 @@ pub(crate) mod innerlude {
     pub use crate::nodes::*;
     pub use crate::properties::*;
     pub use crate::runtime::{Runtime, RuntimeGuard};
+    pub use crate::scheduler::*;
     pub use crate::scopes::*;
     pub use crate::tasks::*;
     pub use crate::virtual_dom::*;
@@ -65,13 +67,13 @@ pub mod prelude {
     pub use crate::innerlude::{
         consume_context, consume_context_from_scope, current_owner, current_scope_id,
         fc_to_builder, generation, has_context, needs_update, needs_update_any, parent_scope,
-        provide_context, provide_root_context, remove_future, schedule_update, schedule_update_any,
-        spawn, spawn_forever, spawn_isomorphic, suspend, try_consume_context, use_after_render,
-        use_before_render, use_drop, use_error_boundary, use_hook, use_hook_with_cleanup,
-        wait_for_next_render, with_owner, AnyValue, Attribute, Component, ComponentFunction,
-        Element, ErrorBoundary, Event, EventHandler, Fragment, HasAttributes, IntoAttributeValue,
-        IntoDynNode, OptionStringFromMarker, Properties, Runtime, RuntimeGuard, ScopeId,
-        ScopeState, SuperFrom, SuperInto, Task, Template, TemplateAttribute, TemplateNode, Throw,
-        VNode, VNodeInner, VirtualDom,
+        provide_context, provide_root_context, queue_effect, remove_future, schedule_update,
+        schedule_update_any, spawn, spawn_forever, spawn_isomorphic, suspend, try_consume_context,
+        use_after_render, use_before_render, use_drop, use_error_boundary, use_hook,
+        use_hook_with_cleanup, wait_for_next_render, with_owner, AnyValue, Attribute, Component,
+        ComponentFunction, Element, ErrorBoundary, Event, EventHandler, Fragment, HasAttributes,
+        IntoAttributeValue, IntoDynNode, OptionStringFromMarker, Properties, Runtime, RuntimeGuard,
+        ScopeId, ScopeState, SuperFrom, SuperInto, Task, Template, TemplateAttribute, TemplateNode,
+        Throw, VNode, VNodeInner, VirtualDom,
     };
 }

packages/core/src/render_signal.rs

@@ -1,4 +1,4 @@
-//! In dioxus, effects are run using normal async functions after a render. [RenderSignalFuture] is a future that resolves after a render has passed.
+//! TODO: We no longer run effects with async tasks. Effects are now their own type of task. We should remove this next breaking release.
 
 use std::cell::RefCell;
 use std::future::Future;

packages/core/src/runtime.rs

@@ -1,3 +1,4 @@
+use crate::innerlude::Effect;
 use crate::{
     innerlude::{LocalTask, SchedulerMsg},
     render_signal::RenderSignal,
@@ -5,6 +6,7 @@ use crate::{
     scopes::ScopeId,
     Task,
 };
+use std::collections::BTreeSet;
 use std::{
     cell::{Cell, Ref, RefCell},
     rc::Rc,
@@ -36,6 +38,9 @@ pub struct Runtime {
 
     // Synchronous tasks need to be run after the next render. The virtual dom stores a list of those tasks to send a signal to them when the next render is done.
     pub(crate) render_signal: RenderSignal,
+
+    // The effects that need to be run after the next render
+    pub(crate) pending_effects: RefCell<BTreeSet<Effect>>,
 }
 
 impl Runtime {
@@ -49,6 +54,7 @@ impl Runtime {
             current_task: Default::default(),
             tasks: Default::default(),
             suspended_tasks: Default::default(),
+            pending_effects: Default::default(),
         })
     }
 
@@ -150,6 +156,19 @@ impl Runtime {
     pub(crate) fn with_scope<R>(scope: ScopeId, f: impl FnOnce(&Scope) -> R) -> Option<R> {
         Self::with(|rt| rt.get_state(scope).map(|sc| f(&sc))).flatten()
     }
+
+    /// Finish a render. This will mark all effects as ready to run and send the render signal.
+    pub(crate) fn finish_render(&self) {
+        // If there are new effects we can run, send a message to the scheduler to run them (after the renderer has applied the mutations)
+        if !self.pending_effects.borrow().is_empty() {
+            self.sender
+                .unbounded_send(SchedulerMsg::EffectQueued)
+                .expect("Scheduler should exist");
+        }
+
+        // And send the render signal
+        self.render_signal.send();
+    }
 }
 
 /// A guard for a new runtime. This must be used to override the current runtime when importing components from a dynamic library that has it's own runtime.

packages/core/src/dirty_scope.rs → packages/core/src/scheduler.rs

@@ -1,32 +1,81 @@
-//! Dioxus resolves scopes in a specific order to avoid unexpected behavior. All tasks are resolved in the order of height. Scopes that are higher up in the tree are resolved first.
-//! When a scope that is higher up in the tree is rerendered, it may drop scopes lower in the tree along with their tasks.
+//! # Dioxus uses a scheduler to run queued work in the correct order.
 //!
-//! ```rust
-//! use dioxus::prelude::*;
+//! ## Goals
+//! We try to prevent three different situations:
+//! 1. Running queued work after it could be dropped. Related issues (https://github.com/DioxusLabs/dioxus/pull/1993)
+//!
+//! User code often assumes that this property is true. For example, if this code reruns the child component after signal is changed to None, it will panic
+//! ```rust, ignore
+//! fn ParentComponent() -> Element {
+//!     let signal: Signal<Option<i32>> = use_signal(None);
 //!
-//! fn app() -> Element {
-//!     let vec = use_signal(|| vec![0; 10]);
 //!     rsx! {
-//!         // If the length of the vec shrinks we need to make sure that the children are dropped along with their tasks the new state of the vec is read
-//!         for idx in 0..vec.len() {
-//!             Child { idx, vec }
+//!         if signal.read().is_some() {
+//!             ChildComponent { signal }
 //!         }
 //!     }
 //! }
 //!
 //! #[component]
-//! fn Child(vec: Signal<Vec<usize>>, idx: usize) -> Element {
-//!     use_hook(move || {
-//!         spawn(async move {
-//!             // If we let this task run after the child is dropped, it will panic.
-//!             println!("Task {}", vec.read()[idx]);
-//!         });
+//! fn ChildComponent(signal: Signal<Option<i32>>) -> Element {
+//!     // It feels safe to assume that signal is some because the parent component checked that it was some
+//!     rsx! { "{signal.read().unwrap()}" }
+//! }
+//! ```
+//!
+//! 2. Running effects before the dom is updated. Related issues (https://github.com/DioxusLabs/dioxus/issues/2307)
+//!
+//! Effects can be used to run code that accesses the DOM directly. They should only run when the DOM is in an updated state. If they are run with an out of date version of the DOM, unexpected behavior can occur.
+//! ```rust, ignore
+//! fn EffectComponent() -> Element {
+//!     let id = use_signal(0);
+//!     use_effect(move || {
+//!         let id = id.read();
+//!         // This will panic if the id is not written to the DOM before the effect is run
+//!         eval(format!(r#"document.getElementById("{id}").innerHTML = "Hello World";"#));
 //!     });
 //!
-//!     rsx! {}
+//!     rsx! {
+//!         div { id: "{id}" }
+//!     }
+//! }
+//!
+//! 3. Observing out of date state. Related issues (https://github.com/DioxusLabs/dioxus/issues/1935)
+//!
+//! Where ever possible, updates should happen in an order that makes it impossible to observe an out of date state.
+//! ```rust, ignore
+//! fn OutOfDateComponent() -> Element {
+//!     let id = use_signal(0);
+//!     // When you read memo, it should **always** be two times the value of id
+//!     let memo = use_memo(move || id() * 2);
+//!     assert_eq!(memo(), id() * 2);
+//!
+//!     // This should be true even if you update the value of id in the middle of the component
+//!     id += 1;
+//!     assert_eq!(memo(), id() * 2);
+//!
+//!     rsx! {
+//!         div { id: "{id}" }
+//!     }
 //! }
 //! ```
+//!
+//! ## Implementation
+//!
+//! There are three different types of queued work that can be run by the virtualdom:
+//! 1. Dirty Scopes:
+//!    Description: When a scope is marked dirty, a rerun of the scope will be scheduled. This will cause the scope to rerun and update the DOM if any changes are detected during the diffing phase.
+//!    Priority: These are the highest priority tasks. Dirty scopes will be rerun in order from the scope closest to the root to the scope furthest from the root. We follow this order to ensure that if a higher component reruns and drops a lower component, the lower component will not be run after it should be dropped.
+//!
+//! 2. Tasks:
+//!    Description: Futures spawned in the dioxus runtime each have an unique task id. When the waker for that future is called, the task is rerun.
+//!    Priority: These are the second highest priority tasks. They are run after all other dirty scopes have been resolved because those dirty scopes may cause children (and the tasks those children own) to drop which should cancel the futures.
+//!
+//! 3. Effects:
+//!    Description: Effects should always run after all changes to the DOM have been applied.
+//!    Priority: These are the lowest priority tasks in the scheduler. They are run after all other dirty scopes and futures have been resolved. Other tasks may cause components to rerun, which would update the DOM. These effects should only run after the DOM has been updated.
 
+use crate::innerlude::Effect;
 use crate::ScopeId;
 use crate::Task;
 use crate::VirtualDom;
@@ -106,6 +155,19 @@ impl VirtualDom {
         Some(task)
     }
 
+    /// Take any effects from the highest scope. This should only be called if there is no pending scope reruns or tasks
+    pub(crate) fn pop_effect(&mut self) -> Option<Effect> {
+        let mut pending_effects = self.runtime.pending_effects.borrow_mut();
+        let mut effect = pending_effects.pop_first()?;
+
+        // If the scope doesn't exist for whatever reason, then we should skip it
+        while !self.scopes.contains(effect.order.id.0) {
+            effect = pending_effects.pop_first()?;
+        }
+
+        Some(effect)
+    }
+
     /// Take any work from the highest scope. This may include rerunning the scope and/or running tasks
     pub(crate) fn pop_work(&mut self) -> Option<Work> {
         let mut dirty_scope = self.dirty_scopes.first();

packages/core/src/scope_context.rs

@@ -265,6 +265,11 @@ impl Scope {
         id
     }
 
+    /// Queue an effect to run after the next render
+    pub fn queue_effect(&self, f: impl FnOnce() + 'static) {
+        Runtime::with(|rt| rt.queue_effect(self.id, f)).expect("Runtime to exist");
+    }
+
     /// Mark this component as suspended on a specific task and then return None
     pub fn suspend(&self, task: Task) -> Option<Element> {
         self.last_suspendable_task.set(Some(task));

packages/core/src/tasks.rs

@@ -1,3 +1,5 @@
+use crate::innerlude::Effect;
+use crate::innerlude::ScopeOrder;
 use crate::innerlude::{remove_future, spawn, Runtime};
 use crate::ScopeId;
 use futures_util::task::ArcWake;
@@ -168,6 +170,19 @@ impl Runtime {
         task_id
     }
 
+    /// Queue an effect to run after the next render
+    pub(crate) fn queue_effect(&self, id: ScopeId, f: impl FnOnce() + 'static) {
+        // Add the effect to the queue of effects to run after the next render for the given scope
+        let mut effects = self.pending_effects.borrow_mut();
+        let scope_order = ScopeOrder::new(id.height(), id);
+        match effects.get(&scope_order) {
+            Some(effects) => effects.push_back(Box::new(f)),
+            None => {
+                effects.insert(Effect::new(scope_order, f));
+            }
+        }
+    }
+
     /// Get the currently running task
     pub fn current_task(&self) -> Option<Task> {
         self.current_task.get()
@@ -282,13 +297,15 @@ impl TaskType {
 /// The type of message that can be sent to the scheduler.
 ///
 /// These messages control how the scheduler will process updates to the UI.
-#[derive(Debug)]
 pub(crate) enum SchedulerMsg {
     /// Immediate updates from Components that mark them as dirty
     Immediate(ScopeId),
 
     /// A task has woken and needs to be progressed
     TaskNotified(Task),
+
+    /// An effect has been queued to run after the next render
+    EffectQueued,
 }
 
 struct LocalTaskHandle {

packages/core/src/virtual_dom.rs

@@ -481,6 +481,7 @@ impl VirtualDom {
                 // The task may be marked dirty at the same time as the scope that owns the task is dropped.
                 self.mark_task_dirty(id);
             }
+            SchedulerMsg::EffectQueued => {}
         };
     }
 
@@ -491,6 +492,7 @@ impl VirtualDom {
             match msg {
                 SchedulerMsg::Immediate(id) => self.mark_dirty(id),
                 SchedulerMsg::TaskNotified(task) => self.mark_task_dirty(task),
+                SchedulerMsg::EffectQueued => {}
             }
         }
     }
@@ -513,21 +515,36 @@ impl VirtualDom {
     fn poll_tasks(&mut self) {
         // Make sure we set the runtime since we're running user code
         let _runtime = RuntimeGuard::new(self.runtime.clone());
-        // Next, run any queued tasks
-        // We choose not to poll the deadline since we complete pretty quickly anyways
-        while let Some(task) = self.pop_task() {
-            // Then poll any tasks that might be pending
-            let mut tasks = task.tasks_queued.into_inner();
-            while let Some(task) = tasks.pop_front() {
-                let _ = self.runtime.handle_task_wakeup(task);
 
-                // Running that task, may mark a scope higher up as dirty. If it does, return from the function early
+        // Keep polling tasks until there are no more effects or tasks to run
+        // Or until we have no more dirty scopes
+        while !self.dirty_tasks.is_empty() || !self.runtime.pending_effects.borrow().is_empty() {
+            // Next, run any queued tasks
+            // We choose not to poll the deadline since we complete pretty quickly anyways
+            while let Some(task) = self.pop_task() {
+                // Then poll any tasks that might be pending
+                let mut tasks = task.tasks_queued.into_inner();
+                while let Some(task) = tasks.pop_front() {
+                    let _ = self.runtime.handle_task_wakeup(task);
+
+                    // Running that task, may mark a scope higher up as dirty. If it does, return from the function early
+                    self.queue_events();
+                    if self.has_dirty_scopes() {
+                        // requeue any remaining tasks
+                        for task in tasks {
+                            self.mark_task_dirty(task);
+                        }
+                        return;
+                    }
+                }
+            }
+
+            // At this point, we have finished running all tasks that are pending and we haven't found any scopes to rerun. This means it is safe to run our lowest priority work: effects
+            while let Some(effect) = self.pop_effect() {
+                effect.run(&self.runtime);
+                // Check if any new scopes are queued for rerun
                 self.queue_events();
                 if self.has_dirty_scopes() {
-                    // requeue any remaining tasks
-                    for task in tasks {
-                        self.mark_task_dirty(task);
-                    }
                     return;
                 }
             }
@@ -633,7 +650,7 @@ impl VirtualDom {
 
         // Process any events that might be pending in the queue
         // Signals marked with .write() need a chance to be handled by the effect driver
-        // This also processes futures which might progress into immediates
+        // This also processes futures which might progress into immediately rerunning a scope
         self.process_events();
 
         // Next, diff any dirty scopes
@@ -658,7 +675,7 @@ impl VirtualDom {
             }
         }
 
-        self.runtime.render_signal.send();
+        self.runtime.finish_render();
     }
 
     /// [`Self::render_immediate`] to a vector of mutations for testing purposes

packages/hooks/src/use_effect.rs

@@ -41,25 +41,29 @@ use crate::use_callback;
 /// ```
 #[track_caller]
 pub fn use_effect(callback: impl FnMut() + 'static) -> Effect {
-    // let mut run_effect = use_hook(|| CopyValue::new(true));
-    // use_hook_did_run(move |did_run| run_effect.set(did_run));
-
     let callback = use_callback(callback);
 
     let location = std::panic::Location::caller();
 
     use_hook(|| {
+        // Inside the effect, we track any reads so that we can rerun the effect if a value the effect reads changes
         let (rc, mut changed) = ReactiveContext::new_with_origin(location);
+        // Spawn a task that will run the effect when:
+        // 1) The component is first run
+        // 2) The effect is rerun due to an async read at any time
+        // 3) The effect is rerun in the same tick that the component is rerun: we need to wait for the component to rerun before we can run the effect again
+        let queue_effect_for_next_render = move || {
+            queue_effect(move || rc.run_in(&*callback));
+        };
+
+        queue_effect_for_next_render();
         spawn(async move {
             loop {
-                // Run the effect
-                rc.run_in(&*callback);
-
                 // Wait for context to change
                 let _ = changed.next().await;
 
-                // Wait for the dom the be finished with sync work
-                wait_for_next_render().await;
+                // Run the effect
+                queue_effect_for_next_render();
             }
         });
         Effect { rc }

packages/hooks/tests/effect.rs

@@ -2,6 +2,7 @@
 use std::cell::RefCell;
 use std::collections::HashMap;
 use std::rc::Rc;
+use std::time::Duration;
 
 use dioxus::prelude::*;
 use dioxus_core::ElementId;
@@ -42,9 +43,63 @@ async fn effects_rerun() {
     );
 
     dom.rebuild_in_place();
-    dom.wait_for_work().await;
+    tokio::select! {
+        _ = dom.wait_for_work() => {}
+        _ = tokio::time::sleep(Duration::from_millis(500)) => panic!("timed out")
+    };
 
     let current_counter = counter.borrow();
     assert_eq!(current_counter.component, 1);
     assert_eq!(current_counter.effect, 1);
 }
+
+// https://github.com/DioxusLabs/dioxus/issues/2347
+// Effects should rerun when the signal changes if there are no changes to the component
+#[tokio::test]
+async fn effects_rerun_without_rerender() {
+    #[derive(Default)]
+    struct RunCounter {
+        component: usize,
+        effect: usize,
+    }
+
+    let counter = Rc::new(RefCell::new(RunCounter::default()));
+    let mut dom = VirtualDom::new_with_props(
+        |counter: Rc<RefCell<RunCounter>>| {
+            counter.borrow_mut().component += 1;
+            println!("component {}", counter.borrow().component);
+
+            let mut signal = use_signal(|| 0);
+            use_effect({
+                to_owned![counter];
+                move || {
+                    counter.borrow_mut().effect += 1;
+                    // This will subscribe the effect to the signal
+                    println!("Signal: {}", signal);
+                }
+            });
+            use_future(move || async move {
+                for i in 0..10 {
+                    tokio::time::sleep(std::time::Duration::from_millis(10)).await;
+                    println!("future {}", i);
+                    signal += 1;
+                }
+            });
+
+            rsx! {
+                div {}
+            }
+        },
+        counter.clone(),
+    );
+
+    dom.rebuild_in_place();
+    tokio::select! {
+        _ = dom.wait_for_work() => {}
+        _ = tokio::time::sleep(Duration::from_millis(500)) => {}
+    };
+
+    let current_counter = counter.borrow();
+    assert_eq!(current_counter.component, 1);
+    assert_eq!(current_counter.effect, 11);
+}