Error reporting QoL

- make Actor::Error Display in addition to Debug - print handle() (and friends) errors using `{:#}` rather than `{:?}` - this hides the backtrace print of anyhow errors - differentiate handle() (and friends) errors while shutting down vs. running and log them with lower severity - make it more clear and less spammy when trying to shut down a system while it is already shutting down
tonarino · Aug 12, 2024 · d7b883a · d7b883a
1 parent 4ce67b5
commit d7b883a
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 42 deletions.
diff --git a/benches/benches.rs b/benches/benches.rs
@@ -8,7 +8,7 @@ struct ChainLink {
 
 impl Actor for ChainLink {
     type Context = Context<Self::Message>;
-    type Error = ();
+    type Error = String;
     type Message = u64;
 
     fn name() -> &'static str {

diff --git a/rustfmt.toml b/rustfmt.toml
@@ -1,4 +1,5 @@
-use_small_heuristics="Max"
+format_strings = true
+use_small_heuristics = "Max"
 imports_granularity = "Crate"
 match_block_trailing_comma = true
 reorder_impl_items = true

diff --git a/src/lib.rs b/src/lib.rs
@@ -15,14 +15,14 @@
 //! struct TestActor {}
 //! impl Actor for TestActor {
 //!     type Context = Context<Self::Message>;
-//!     type Error = ();
+//!     type Error = String;
 //!     type Message = usize;
 //!
 //!     fn name() -> &'static str {
 //!         "TestActor"
 //!     }
 //!
-//!     fn handle(&mut self, _context: &mut Self::Context, message: Self::Message) -> Result<(), ()> {
+//!     fn handle(&mut self, _context: &mut Self::Context, message: Self::Message) -> Result<(), String> {
 //!         println!("message: {}", message);
 //!
 //!         Ok(())
@@ -322,7 +322,8 @@ impl From<usize> for Capacity {
 /// A builder for configuring [`Actor`] spawning.
 /// You can specify your own [`Addr`] for the Actor, or let the system create
 /// a new address with either provided or default capacity.
-#[must_use = "You must call .with_addr(), .with_capacity(), or .with_default_capacity() to configure this builder"]
+#[must_use = "You must call .with_addr(), .with_capacity(), or .with_default_capacity() to \
+              configure this builder"]
 pub struct SpawnBuilderWithoutAddress<'a, A: Actor, F: FnOnce() -> A> {
     system: &'a mut System,
     factory: F,
@@ -573,35 +574,52 @@ impl System {
                 },
                 Received::Message(msg) => {
                     trace!("[{}] message received by {}", system_handle.name, A::name());
-                    if let Err(err) = actor.handle(context, msg) {
-                        error!(
-                            "[{}] {} handle error: {:?}, shutting down.",
-                            system_handle.name,
-                            A::name(),
-                            err
-                        );
-                        let _ = system_handle.shutdown();
-
+                    if let Err(error) = actor.handle(context, msg) {
+                        Self::report_error_shutdown(system_handle, A::name(), "handle", error);
                         return;
                     }
                 },
                 Received::Timeout => {
                     let deadline = context.receive_deadline.take().expect("implied by timeout");
-                    if let Err(err) = actor.deadline_passed(context, deadline) {
-                        error!(
-                            "[{}] {} deadline_passed error: {:?}, shutting down.",
-                            system_handle.name,
+                    if let Err(error) = actor.deadline_passed(context, deadline) {
+                        Self::report_error_shutdown(
+                            system_handle,
                             A::name(),
-                            err
+                            "deadline_passed",
+                            error,
                         );
-                        let _ = system_handle.shutdown();
-
                         return;
                     }
                 },
             }
         }
     }
+
+    fn report_error_shutdown(
+        system_handle: &SystemHandle,
+        actor_name: &str,
+        method_name: &str,
+        error: impl std::fmt::Display,
+    ) {
+        let is_running = match *system_handle.system_state.read() {
+            SystemState::Running => true,
+            SystemState::ShuttingDown | SystemState::Stopped => false,
+        };
+
+        let system_name = &system_handle.name;
+
+        // Note that the system may have transitioned from running to stopping (but not the other
+        // way around) in the mean time. Slightly imprecise log and an extra no-op call is fine.
+        if is_running {
+            error!("[{system_name}] {actor_name} {method_name} error: {error:#}, shutting down.");
+            let _ = system_handle.shutdown();
+        } else {
+            warn!(
+                "[{system_name}] {actor_name} {method_name} error {error:#} while shutting down, \
+                 ignoring."
+            );
+        }
+    }
 }
 
 impl Drop for System {
@@ -623,29 +641,30 @@ impl SystemHandle {
     pub fn shutdown(&self) -> Result<(), ActorError> {
         let current_thread = thread::current();
         let current_thread_name = current_thread.name().unwrap_or("Unknown thread id");
-        info!("Thread [{}] shutting down the actor system", current_thread_name);
 
         // Use an inner scope to prevent holding the lock for the duration of shutdown
         {
             let mut system_state_lock = self.system_state.write();
 
             match *system_state_lock {
                 SystemState::ShuttingDown | SystemState::Stopped => {
-                    debug!("Thread [{}] called system.shutdown() but the system is already shutting down or stopped", current_thread_name);
+                    debug!(
+                        "Thread [{}] called system.shutdown() but the system is already shutting \
+                         down or stopped",
+                        current_thread_name
+                    );
                     return Ok(());
                 },
                 SystemState::Running => {
-                    debug!(
-                        "Thread [{}] setting the system_state value to ShuttingDown",
-                        current_thread_name
+                    info!(
+                        "Thread [{}] shutting down the actor system {}",
+                        current_thread_name, self.name
                     );
                     *system_state_lock = SystemState::ShuttingDown;
                 },
             }
         }
 
-        info!("[{}] system shutting down.", self.name);
-
         if let Some(callback) = self.callbacks.preshutdown.as_ref() {
             info!("[{}] calling pre-shutdown callback.", self.name);
             if let Err(err) = callback() {
@@ -784,7 +803,7 @@ pub trait Actor {
     // 'static required to create trait object in Addr, https://stackoverflow.com/q/29740488/4345715
     type Message: Send + 'static;
     /// The type to return on error in the handle method.
-    type Error: std::fmt::Debug;
+    type Error: std::fmt::Debug + std::fmt::Display;
     /// What kind of context this actor accepts. Usually [`Context<Self::Message>`].
     type Context;
 
@@ -827,13 +846,13 @@ pub trait Actor {
     /// # struct TickingActor;
     /// impl Actor for TickingActor {
     /// #    type Context = Context<Self::Message>;
-    /// #    type Error = ();
+    /// #    type Error = String;
     /// #    type Message = ();
     /// #    fn name() -> &'static str { "TickingActor" }
-    /// #    fn handle(&mut self, _: &mut Self::Context, _: ()) -> Result<(), ()> { Ok(()) }
+    /// #    fn handle(&mut self, _: &mut Self::Context, _: ()) -> Result<(), String> { Ok(()) }
     ///     // ...
     ///
-    ///     fn deadline_passed(&mut self, context: &mut Self::Context, deadline: Instant) -> Result<(), ()> {
+    ///     fn deadline_passed(&mut self, context: &mut Self::Context, deadline: Instant) -> Result<(), String> {
     ///         // do_periodic_housekeeping();
     ///
     ///         // A: Schedule one second from now (even if delayed); drifting tick.
@@ -1045,14 +1064,14 @@ mod tests {
     struct TestActor;
     impl Actor for TestActor {
         type Context = Context<Self::Message>;
-        type Error = ();
+        type Error = String;
         type Message = usize;
 
         fn name() -> &'static str {
             "TestActor"
         }
 
-        fn handle(&mut self, _: &mut Self::Context, message: usize) -> Result<(), ()> {
+        fn handle(&mut self, _: &mut Self::Context, message: usize) -> Result<(), String> {
             println!("message: {}", message);
 
             Ok(())
@@ -1106,14 +1125,14 @@ mod tests {
         }
         impl Actor for LocalActor {
             type Context = Context<Self::Message>;
-            type Error = ();
+            type Error = String;
             type Message = ();
 
             fn name() -> &'static str {
                 "LocalActor"
             }
 
-            fn handle(&mut self, _: &mut Self::Context, _: ()) -> Result<(), ()> {
+            fn handle(&mut self, _: &mut Self::Context, _: ()) -> Result<(), String> {
                 Ok(())
             }
 
@@ -1143,22 +1162,26 @@ mod tests {
 
         impl Actor for TimeoutActor {
             type Context = Context<Self::Message>;
-            type Error = ();
+            type Error = String;
             type Message = Option<Instant>;
 
             fn name() -> &'static str {
                 "TimeoutActor"
             }
 
-            fn handle(&mut self, ctx: &mut Self::Context, msg: Self::Message) -> Result<(), ()> {
+            fn handle(
+                &mut self,
+                ctx: &mut Self::Context,
+                msg: Self::Message,
+            ) -> Result<(), String> {
                 self.handle_count.fetch_add(1, Ordering::SeqCst);
                 if msg.is_some() {
                     ctx.receive_deadline = msg;
                 }
                 Ok(())
             }
 
-            fn deadline_passed(&mut self, _: &mut Self::Context, _: Instant) -> Result<(), ()> {
+            fn deadline_passed(&mut self, _: &mut Self::Context, _: Instant) -> Result<(), String> {
                 self.timeout_count.fetch_add(1, Ordering::SeqCst);
                 Ok(())
             }
@@ -1238,7 +1261,7 @@ mod tests {
 
         impl Actor for PriorityActor {
             type Context = Context<Self::Message>;
-            type Error = ();
+            type Error = String;
             type Message = usize;
 
             fn handle(

diff --git a/src/timed.rs b/src/timed.rs
@@ -282,14 +282,14 @@ mod tests {
 
     impl Actor for TimedTestActor {
         type Context = TimedContext<Self::Message>;
-        type Error = ();
+        type Error = String;
         type Message = usize;
 
         fn name() -> &'static str {
             "TimedTestActor"
         }
 
-        fn handle(&mut self, context: &mut Self::Context, message: usize) -> Result<(), ()> {
+        fn handle(&mut self, context: &mut Self::Context, message: usize) -> Result<(), String> {
             {
                 let mut guard = self.received.lock().unwrap();
                 guard.push(message);