feat: better error handling for ScopedFuture (#1810)

Author: gbj

leptos_reactive/src/runtime.rs

@@ -26,6 +26,7 @@ use std::{
     rc::Rc,
     task::Poll,
 };
+use thiserror::Error;
 
 pub(crate) type PinnedFuture<T> = Pin<Box<dyn Future<Output = T>>>;
 
@@ -97,9 +98,6 @@ pub struct Owner(pub(crate) NodeId);
 
 impl Owner {
     /// Returns the current reactive owner.
-    ///
-    /// ## Panics
-    /// Panics if there is no current reactive runtime.
     pub fn current() -> Option<Owner> {
         with_runtime(|runtime| runtime.owner.get())
             .ok()
@@ -686,16 +684,19 @@ impl Debug for Runtime {
     instrument(level = "trace", skip_all,)
 )]
 #[inline(always)] // it monomorphizes anyway
-pub(crate) fn with_runtime<T>(f: impl FnOnce(&Runtime) -> T) -> Result<T, ()> {
+pub(crate) fn with_runtime<T>(
+    f: impl FnOnce(&Runtime) -> T,
+) -> Result<T, ReactiveSystemError> {
     // in the browser, everything should exist under one runtime
     cfg_if! {
         if #[cfg(any(feature = "csr", feature = "hydrate"))] {
             Ok(RUNTIME.with(|runtime| f(runtime)))
         } else {
             RUNTIMES.with(|runtimes| {
                 let runtimes = runtimes.borrow();
-                match runtimes.get(Runtime::current()) {
-                    None => Err(()),
+                let rt = Runtime::current();
+                match runtimes.get(rt) {
+                    None => Err(ReactiveSystemError::RuntimeDisposed(rt)),
                     Some(runtime) => Ok(f(runtime))
                 }
             })
@@ -820,38 +821,49 @@ where
 /// ## Panics
 /// Panics if there is no current reactive runtime.
 pub fn with_owner<T>(owner: Owner, f: impl FnOnce() -> T) -> T {
-    try_with_owner(owner, f)
-        .expect("runtime/scope should be alive when with_owner runs")
+    try_with_owner(owner, f).unwrap()
+}
+
+#[derive(Error, Debug)]
+pub enum ReactiveSystemError {
+    #[error("Runtime {0:?} has been disposed.")]
+    RuntimeDisposed(RuntimeId),
+    #[error("Owner {0:?} has been disposed.")]
+    OwnerDisposed(Owner),
+    #[error("Error borrowing runtime.nodes {0:?}")]
+    Borrow(std::cell::BorrowError),
 }
 
 /// Runs the given code with the given reactive owner.
-pub fn try_with_owner<T>(owner: Owner, f: impl FnOnce() -> T) -> Option<T> {
+pub fn try_with_owner<T>(
+    owner: Owner,
+    f: impl FnOnce() -> T,
+) -> Result<T, ReactiveSystemError> {
     with_runtime(|runtime| {
-        runtime
-            .nodes
-            .try_borrow()
-            .map(|nodes| nodes.contains_key(owner.0))
-            .map(|scope_exists| {
-                scope_exists.then(|| {
-                    let prev_observer = runtime.observer.take();
-                    let prev_owner = runtime.owner.take();
+        let scope_exists = {
+            let nodes = runtime
+                .nodes
+                .try_borrow()
+                .map_err(ReactiveSystemError::Borrow)?;
+            nodes.contains_key(owner.0)
+        };
+        if scope_exists {
+            let prev_observer = runtime.observer.take();
+            let prev_owner = runtime.owner.take();
 
-                    runtime.owner.set(Some(owner.0));
-                    runtime.observer.set(Some(owner.0));
+            runtime.owner.set(Some(owner.0));
+            runtime.observer.set(Some(owner.0));
 
-                    let v = f();
+            let v = f();
 
-                    runtime.observer.set(prev_observer);
-                    runtime.owner.set(prev_owner);
+            runtime.observer.set(prev_observer);
+            runtime.owner.set(prev_owner);
 
-                    v
-                })
-            })
-            .ok()
-            .flatten()
-    })
-    .ok()
-    .flatten()
+            Ok(v)
+        } else {
+            Err(ReactiveSystemError::OwnerDisposed(owner))
+        }
+    })?
 }
 
 /// Runs the given function as a child of the current Owner, once.
@@ -1494,23 +1506,25 @@ pub struct ScopedFuture<Fut: Future> {
     future: Fut,
 }
 
+/// Errors that can occur when trying to spawn a [`ScopedFuture`].
+#[derive(Error, Debug, Clone)]
+pub enum ScopedFutureError {
+    #[error(
+        "Tried to spawn a scoped Future without a current reactive Owner."
+    )]
+    NoCurrentOwner,
+}
+
 impl<Fut: Future + 'static> Future for ScopedFuture<Fut> {
     type Output = Option<Fut::Output>;
 
     fn poll(
         self: Pin<&mut Self>,
         cx: &mut std::task::Context<'_>,
     ) -> Poll<Self::Output> {
-        // TODO: we need to think about how to make this
-        // not panic for scopes that have been cleaned up...
-        // or perhaps we can force the scope to not be cleaned
-        // up until all futures that have a handle to them are
-        // dropped...
-
         let this = self.project();
 
-        if let Some(poll) = try_with_owner(*this.owner, || this.future.poll(cx))
-        {
+        if let Ok(poll) = try_with_owner(*this.owner, || this.future.poll(cx)) {
             match poll {
                 Poll::Ready(res) => Poll::Ready(Some(res)),
                 Poll::Pending => Poll::Pending,
@@ -1529,33 +1543,31 @@ impl<Fut: Future> ScopedFuture<Fut> {
     }
 
     /// Runs the future in the current [`Owner`]'s scope context.
-    ///
-    /// # Panics
-    /// Panics if there is no current [`Owner`] context available.
     #[track_caller]
-    pub fn new_current(fut: Fut) -> Self {
-        Self {
-            owner: Owner::current().expect(
-                "`ScopedFuture::new_current()` to be called within an `Owner` \
-                 context",
-            ),
-            future: fut,
-        }
+    pub fn new_current(fut: Fut) -> Result<Self, ScopedFutureError> {
+        Owner::current()
+            .map(|owner| Self { owner, future: fut })
+            .ok_or(ScopedFutureError::NoCurrentOwner)
     }
 }
 
 /// Runs a future that has access to the provided [`Owner`]'s
 /// scope context.
+#[track_caller]
 pub fn spawn_local_with_owner(
     owner: Owner,
     fut: impl Future<Output = ()> + 'static,
 ) {
     let scoped_future = ScopedFuture::new(owner, fut);
+    #[cfg(debug_assertions)]
+    let loc = std::panic::Location::caller();
 
     crate::spawn_local(async move {
         if scoped_future.await.is_none() {
-            // TODO: should we warn here?
-            //     /* warning message */
+            crate::macros::debug_warn!(
+                "`spawn_local_with_owner` called at {loc} returned `None`, \
+                 i.e., its Owner was disposed before the `Future` resolved."
+            );
         }
     });
 }
@@ -1566,15 +1578,23 @@ pub fn spawn_local_with_owner(
 /// # Panics
 /// Panics if there is no [`Owner`] context available.
 #[track_caller]
-pub fn spawn_local_with_current_owner(fut: impl Future<Output = ()> + 'static) {
-    let scoped_future = ScopedFuture::new_current(fut);
+pub fn spawn_local_with_current_owner(
+    fut: impl Future<Output = ()> + 'static,
+) -> Result<(), ScopedFutureError> {
+    let scoped_future = ScopedFuture::new_current(fut)?;
+    #[cfg(debug_assertions)]
+    let loc = std::panic::Location::caller();
 
     crate::spawn_local(async move {
         if scoped_future.await.is_none() {
-            // TODO: should we warn here?
-            //     /* warning message */
+            crate::macros::debug_warn!(
+                "`spawn_local_with_owner` called at {loc} returned `None`, \
+                 i.e., its Owner was disposed before the `Future` resolved."
+            );
         }
     });
+
+    Ok(())
 }
 
 /// Runs a future that has access to the provided [`Owner`]'s
@@ -1616,12 +1636,14 @@ pub fn try_spawn_local_with_owner(
 pub fn try_spawn_local_with_current_owner(
     fut: impl Future<Output = ()> + 'static,
     on_cancelled: impl FnOnce() + 'static,
-) {
-    let scoped_future = ScopedFuture::new_current(fut);
+) -> Result<(), ScopedFutureError> {
+    let scoped_future = ScopedFuture::new_current(fut)?;
 
     crate::spawn_local(async move {
         if scoped_future.await.is_none() {
             on_cancelled();
         }
     });
+
+    Ok(())
 }