Fix service framework (#31)

 - Fix deadlock when accessing the ServiceManager parameter in start() method of a Service
 - Known bug: Deadlock still happens when a service accesses itself through the ServiceManager on start

Author: Kitt3120

src/bot.rs

@@ -41,7 +41,7 @@ impl BotBuilder {
 
 pub struct Bot {
     pub name: String,
-    pub service_manager: Arc<RwLock<ServiceManager>>,
+    pub service_manager: Arc<ServiceManager>,
 }
 
 impl Bot {
@@ -52,15 +52,15 @@ impl Bot {
     //TODO: When Rust allows async trait methods to be object-safe, refactor this to use async instead of returning a future
     pub fn start(&mut self) -> PinnedBoxedFuture<'_, ()> {
         Box::pin(async move {
-            self.service_manager.write().await.start_services().await;
+            self.service_manager.start_services().await;
             //TODO: Potential for further initialization here, like modules
         })
     }
 
     //TODO: When Rust allows async trait methods to be object-safe, refactor this to use async instead of returning a future
     pub fn stop(&mut self) -> PinnedBoxedFuture<'_, ()> {
         Box::pin(async move {
-            self.service_manager.write().await.stop_services().await;
+            self.service_manager.stop_services().await;
             //TODO: Potential for further deinitialization here, like modules
         })
     }

src/lib.rs

@@ -36,17 +36,15 @@ pub async fn run(mut bot: Bot) {
         }
     };
 
-    let service_manager = bot.service_manager.read().await;
-    if service_manager.overall_status().await != OverallStatus::Healthy {
-        let status_tree = service_manager.status_tree().await;
+    if bot.service_manager.overall_status().await != OverallStatus::Healthy {
+        let status_tree = bot.service_manager.status_tree().await;
 
         error!("{} is not healthy! Some essential services did not start up successfully. Please check the logs.\nService status tree:\n{}\n{} will exit.",
         bot.name,
         status_tree,
         bot.name);
         return;
     }
-    drop(service_manager);
 
     info!("{} is alive", bot.name,);
 

src/service.rs

@@ -17,8 +17,10 @@ use crate::setlock::SetLock;
 
 pub mod discord;
 
-pub type PinnedBoxedFuture<'a, T> = Pin<Box<dyn Future<Output = T> + 'a>>;
+pub type BoxedFuture<'a, T> = Box<dyn Future<Output = T> + 'a>;
+pub type BoxedFutureResult<'a, T> = BoxedFuture<'a, Result<T, Box<dyn Error + Send + Sync>>>;
 
+pub type PinnedBoxedFuture<'a, T> = Pin<Box<dyn Future<Output = T> + 'a>>;
 pub type PinnedBoxedFutureResult<'a, T> =
     PinnedBoxedFuture<'a, Result<T, Box<dyn Error + Send + Sync>>>;
 
@@ -28,8 +30,8 @@ pub enum Status {
     Stopped,
     Starting,
     Stopping,
-    FailedStarting(Box<dyn Error + Send + Sync>),
-    FailedStopping(Box<dyn Error + Send + Sync>),
+    FailedToStart(Box<dyn Error + Send + Sync>), //TODO: Test out if it'd be better to use a String instead
+    FailedToStop(Box<dyn Error + Send + Sync>),
     RuntimeError(Box<dyn Error + Send + Sync>),
 }
 
@@ -40,8 +42,8 @@ impl Display for Status {
             Status::Stopped => write!(f, "Stopped"),
             Status::Starting => write!(f, "Starting"),
             Status::Stopping => write!(f, "Stopping"),
-            Status::FailedStarting(error) => write!(f, "Failed to start: {}", error),
-            Status::FailedStopping(error) => write!(f, "Failed to stop: {}", error),
+            Status::FailedToStart(error) => write!(f, "Failed to start: {}", error),
+            Status::FailedToStop(error) => write!(f, "Failed to stop: {}", error),
             Status::RuntimeError(error) => write!(f, "Runtime error: {}", error),
         }
     }
@@ -55,8 +57,8 @@ impl PartialEq for Status {
                 | (Status::Stopped, Status::Stopped)
                 | (Status::Starting, Status::Starting)
                 | (Status::Stopping, Status::Stopping)
-                | (Status::FailedStarting(_), Status::FailedStarting(_))
-                | (Status::FailedStopping(_), Status::FailedStopping(_))
+                | (Status::FailedToStart(_), Status::FailedToStart(_))
+                | (Status::FailedToStop(_), Status::FailedToStop(_))
                 | (Status::RuntimeError(_), Status::RuntimeError(_))
         )
     }
@@ -146,19 +148,13 @@ impl Hash for ServiceInfo {
 //TODO: When Rust allows async trait methods to be object-safe, refactor this to use async instead of returning a PinnedBoxedFutureResult
 pub trait Service: DowncastSync {
     fn info(&self) -> &ServiceInfo;
-    fn start(
-        &mut self,
-        service_manager: Arc<RwLock<ServiceManager>>,
-    ) -> PinnedBoxedFutureResult<'_, ()>;
+    fn start(&mut self, service_manager: Arc<ServiceManager>) -> PinnedBoxedFutureResult<'_, ()>;
     fn stop(&mut self) -> PinnedBoxedFutureResult<'_, ()>;
 
     // Used for downcasting in get_service method of ServiceManager
     //fn as_any_arc(&self) -> Arc<dyn Any + Send + Sync>;
 
-    fn wrapped_start(
-        &mut self,
-        service_manager: Arc<RwLock<ServiceManager>>,
-    ) -> PinnedBoxedFuture<()> {
+    fn wrapped_start(&mut self, service_manager: Arc<ServiceManager>) -> PinnedBoxedFuture<()> {
         Box::pin(async move {
             let mut status = self.info().status.write().await;
 
@@ -181,7 +177,7 @@ pub trait Service: DowncastSync {
                 }
                 Err(error) => {
                     error!("Failed to start service {}: {}", self.info().name, error);
-                    self.info().set_status(Status::FailedStarting(error)).await;
+                    self.info().set_status(Status::FailedToStart(error)).await;
                 }
             }
         })
@@ -210,7 +206,7 @@ pub trait Service: DowncastSync {
                 }
                 Err(error) => {
                     error!("Failed to stop service {}: {}", self.info().name, error);
-                    self.info().set_status(Status::FailedStopping(error)).await;
+                    self.info().set_status(Status::FailedToStop(error)).await;
                 }
             }
         })
@@ -288,22 +284,15 @@ impl ServiceManagerBuilder {
         self
     }
 
-    pub async fn build(self) -> Arc<RwLock<ServiceManager>> {
+    pub async fn build(self) -> Arc<ServiceManager> {
         let service_manager = ServiceManager {
             services: self.services,
             arc: RwLock::new(SetLock::new()),
         };
 
-        let self_arc = Arc::new(RwLock::new(service_manager));
+        let self_arc = Arc::new(service_manager);
 
-        match self_arc
-            .write()
-            .await
-            .arc
-            .write()
-            .await
-            .set(Arc::clone(&self_arc))
-        {
+        match self_arc.arc.write().await.set(Arc::clone(&self_arc)) {
             Ok(()) => {}
             Err(err) => {
                 panic!(
@@ -319,7 +308,7 @@ impl ServiceManagerBuilder {
 
 pub struct ServiceManager {
     pub services: Vec<Arc<RwLock<dyn Service>>>,
-    arc: RwLock<SetLock<Arc<RwLock<Self>>>>,
+    arc: RwLock<SetLock<Arc<Self>>>,
 }
 
 impl ServiceManager {
@@ -418,8 +407,8 @@ impl ServiceManager {
                                 .push_str(&format!(" - {}: {}\n", info.name, status));
                         }
                     },
-                    Status::FailedStarting(_)
-                    | Status::FailedStopping(_)
+                    Status::FailedToStart(_)
+                    | Status::FailedToStop(_)
                     | Status::RuntimeError(_) => match priority {
                         Priority::Essential => {
                             failed_essentials.push_str(&format!(" - {}: {}\n", info.name, status));

src/service/discord.rs

@@ -14,6 +14,7 @@ use serenity::{
 };
 use std::{sync::Arc, time::Duration};
 use tokio::{
+    spawn,
     sync::{Mutex, Notify, RwLock},
     task::JoinHandle,
     time::{sleep, timeout},
@@ -56,10 +57,7 @@ impl Service for DiscordService {
         &self.info
     }
 
-    fn start(
-        &mut self,
-        _service_manager: Arc<RwLock<ServiceManager>>,
-    ) -> PinnedBoxedFutureResult<'_, ()> {
+    fn start(&mut self, _service_manager: Arc<ServiceManager>) -> PinnedBoxedFutureResult<'_, ()> {
         Box::pin(async move {
             let framework = StandardFramework::new();
             framework.configure(Configuration::new().prefix("!"));
@@ -101,7 +99,7 @@ impl Service for DiscordService {
             }
 
             info!("Connecting to Discord");
-            let client_handle = tokio::spawn(async move { client.start().await });
+            let client_handle = spawn(async move { client.start().await });
 
             // This prevents waiting for the timeout if the client fails immediately
             // TODO: Optimize this, as it will currently add 1000mqs to the startup time