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

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