From 3434c7ac3b7ee19d52fabf93f4ed4dd87279fc6d Mon Sep 17 00:00:00 2001 From: Torben Schweren Date: Sat, 5 Oct 2024 21:06:14 +0200 Subject: [PATCH] refactor: use OnceLock instead of SetLock - Use OnceLock where SetLock was previously used - Remove SetLock --- Cargo.lock | 53 ++++++++++------------- src/event/event_repeater.rs | 29 +++++-------- src/lib.rs | 1 - src/service/discord.rs | 79 +++++++++++++++++++--------------- src/service/service_manager.rs | 23 ++++------ src/setlock.rs | 70 ------------------------------ 6 files changed, 86 insertions(+), 169 deletions(-) delete mode 100644 src/setlock.rs diff --git a/Cargo.lock b/Cargo.lock index 908265d..e6b5872 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -199,9 +199,9 @@ dependencies = [ [[package]] name = "cc" -version = "1.1.24" +version = "1.1.25" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "812acba72f0a070b003d3697490d2b55b837230ae7c6c6497f05cc2ddbb8d938" +checksum = "e8d9e0b4957f635b8d3da819d0db5603620467ecf1f692d22a8c2717ce27e6d8" dependencies = [ "shlex", ] @@ -632,9 +632,9 @@ dependencies = [ [[package]] name = "futures" -version = "0.3.30" +version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "645c6916888f6cb6350d2550b80fb63e734897a8498abe35cfb732b6487804b0" +checksum = "65bc07b1a8bc7c85c5f2e110c476c7389b4554ba72af57d8445ea63a576b0876" dependencies = [ "futures-channel", "futures-core", @@ -646,9 +646,9 @@ dependencies = [ [[package]] name = "futures-channel" -version = "0.3.30" +version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "eac8f7d7865dcb88bd4373ab671c8cf4508703796caa2b1985a9ca867b3fcb78" +checksum = "2dff15bf788c671c1934e366d07e30c1814a8ef514e1af724a602e8a2fbe1b10" dependencies = [ "futures-core", "futures-sink", @@ -656,15 +656,15 @@ dependencies = [ [[package]] name = "futures-core" -version = "0.3.30" +version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dfc6580bb841c5a68e9ef15c77ccc837b40a7504914d52e47b8b0e9bbda25a1d" +checksum = "05f29059c0c2090612e8d742178b0580d2dc940c837851ad723096f87af6663e" [[package]] name = "futures-executor" -version = "0.3.30" +version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a576fc72ae164fca6b9db127eaa9a9dda0d61316034f33a0a0d4eda41f02b01d" +checksum = "1e28d1d997f585e54aebc3f97d39e72338912123a67330d723fdbb564d646c9f" dependencies = [ "futures-core", "futures-task", @@ -684,15 +684,15 @@ dependencies = [ [[package]] name = "futures-io" -version = "0.3.30" +version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a44623e20b9681a318efdd71c299b6b222ed6f231972bfe2f224ebad6311f0c1" +checksum = "9e5c1b78ca4aae1ac06c48a526a655760685149f0d465d21f37abfe57ce075c6" [[package]] name = "futures-macro" -version = "0.3.30" +version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "87750cf4b7a4c0625b1529e4c543c2182106e4dedc60a2a6455e00d212c489ac" +checksum = "162ee34ebcb7c64a8abebc059ce0fee27c2262618d7b60ed8faf72fef13c3650" dependencies = [ "proc-macro2", "quote", @@ -701,21 +701,21 @@ dependencies = [ [[package]] name = "futures-sink" -version = "0.3.30" +version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9fb8e00e87438d937621c1c6269e53f536c14d3fbd6a042bb24879e57d474fb5" +checksum = "e575fab7d1e0dcb8d0c7bcf9a63ee213816ab51902e6d244a95819acacf1d4f7" [[package]] name = "futures-task" -version = "0.3.30" +version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "38d84fa142264698cdce1a9f9172cf383a0c82de1bddcf3092901442c4097004" +checksum = "f90f7dce0722e95104fcb095585910c0977252f286e354b5e3bd38902cd99988" [[package]] name = "futures-util" -version = "0.3.30" +version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3d6401deb83407ab3da39eba7e33987a73c3df0c82b4bb5813ee871c19c41d48" +checksum = "9fa08315bb612088cc391249efdc3bc77536f16c91f6cf495e6fbe85b20a4a81" dependencies = [ "futures-channel", "futures-core", @@ -1361,12 +1361,9 @@ dependencies = [ [[package]] name = "once_cell" -version = "1.20.1" +version = "1.20.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "82881c4be219ab5faaf2ad5e5e5ecdff8c66bd7402ca3160975c93b24961afd1" -dependencies = [ - "portable-atomic", -] +checksum = "1261fe7e33c73b354eab43b1273a57c8f967d0391e80353e51f764ac02cf6775" [[package]] name = "openssl" @@ -1507,12 +1504,6 @@ version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "953ec861398dccce10c670dfeaf3ec4911ca479e9c02154b3a215178c5f566f2" -[[package]] -name = "portable-atomic" -version = "1.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cc9c68a3f6da06753e9335d63e27f6b9754dd1920d941135b7ea8224f141adb2" - [[package]] name = "powerfmt" version = "0.2.0" diff --git a/src/event/event_repeater.rs b/src/event/event_repeater.rs index 7d3a06a..ec58366 100644 --- a/src/event/event_repeater.rs +++ b/src/event/event_repeater.rs @@ -1,14 +1,12 @@ use log::error; use std::{ collections::HashMap, - sync::{Arc, Weak}, + sync::{Arc, OnceLock, Weak}, }; use thiserror::Error; use tokio::{sync::Mutex, task::JoinHandle}; use uuid::Uuid; -use crate::setlock::{SetLock, SetLockError}; - use super::{Event, Subscription}; #[derive(Debug, Error)] @@ -53,7 +51,7 @@ where T: Send + Sync + 'static, { pub event: Event, - weak: Mutex>>, + weak: OnceLock>, subscriptions: Mutex)>>, } @@ -68,7 +66,7 @@ where { let event = Event::new(name); let event_repeater = Self { - weak: Mutex::new(SetLock::new()), + weak: OnceLock::new(), event, subscriptions: Mutex::new(HashMap::new()), }; @@ -76,17 +74,13 @@ where let arc = Arc::new(event_repeater); let weak = Arc::downgrade(&arc); - let result = arc.weak.lock().await.set(weak); - if let Err(err) = result { - match err { - SetLockError::AlreadySet => { - error!("Failed to set EventRepeater {}'s Weak self-reference because it was already set. This should never happen. Shutting down ungracefully to prevent further undefined behavior.", arc.event.name); - unreachable!( - "Unable to set EventRepeater {}'s Weak self-reference because it was already set.", - arc.event.name - ); - } - } + let result = arc.weak.set(weak); + if result.is_err() { + error!("Failed to set EventRepeater {}'s Weak self-reference because it was already set. This should never happen. Shutting down ungracefully to prevent further undefined behavior.", arc.event.name); + unreachable!( + "Unable to set EventRepeater {}'s Weak self-reference because it was already set.", + arc.event.name + ); } arc @@ -97,8 +91,7 @@ where } pub async fn attach(&self, event: &Event, buffer: usize) -> Result<(), AttachError> { - let lock = self.weak.lock().await; - let weak = match lock.get() { + let weak = match self.weak.get() { Some(weak) => weak, None => { return Err(AttachError::NotInitialized { diff --git a/src/lib.rs b/src/lib.rs index dc2f19b..16d59c5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -8,7 +8,6 @@ pub mod config; pub mod event; pub mod log; pub mod service; -pub mod setlock; pub fn is_debug() -> bool { cfg!(debug_assertions) diff --git a/src/service/discord.rs b/src/service/discord.rs index 47ec3d2..5825363 100644 --- a/src/service/discord.rs +++ b/src/service/discord.rs @@ -1,7 +1,5 @@ -use crate::setlock::SetLock; - use super::{types::LifetimedPinnedBoxedFutureResult, Priority, Service, ServiceInfo, ServiceManager}; -use log::{error, info}; +use log::{error, info, warn}; use serenity::{ all::{GatewayIntents, Ready}, async_trait, @@ -12,7 +10,10 @@ use serenity::{ prelude::TypeMap, Client, Error, }; -use std::{sync::Arc, time::Duration}; +use std::{ + sync::{Arc, OnceLock}, + time::Duration, +}; use tokio::{ select, spawn, sync::{Mutex, Notify, RwLock}, @@ -24,14 +25,14 @@ use tokio::{ pub struct DiscordService { info: ServiceInfo, discord_token: String, - pub ready: Arc>>, + pub ready: Arc>, client_handle: Option>>, - pub cache: SetLock>, - pub data: SetLock>>, - pub http: SetLock>, - pub shard_manager: SetLock>, - pub voice_manager: SetLock>, - pub ws_url: SetLock>>, + pub cache: OnceLock>, + pub data: OnceLock>>, + pub http: OnceLock>, + pub shard_manager: OnceLock>, + pub voice_manager: OnceLock>, + pub ws_url: OnceLock>>, } impl DiscordService { @@ -39,14 +40,14 @@ impl DiscordService { Self { info: ServiceInfo::new("lum_builtin_discord", "Discord", Priority::Essential), discord_token: discord_token.to_string(), - ready: Arc::new(Mutex::new(SetLock::new())), + ready: Arc::new(OnceLock::new()), client_handle: None, - cache: SetLock::new(), - data: SetLock::new(), - http: SetLock::new(), - shard_manager: SetLock::new(), - voice_manager: SetLock::new(), - ws_url: SetLock::new(), + cache: OnceLock::new(), + data: OnceLock::new(), + http: OnceLock::new(), + shard_manager: OnceLock::new(), + voice_manager: OnceLock::new(), + ws_url: OnceLock::new(), } } } @@ -71,30 +72,38 @@ impl Service for DiscordService { )) .await?; - if let Err(error) = self.cache.set(Arc::clone(&client.cache)) { - return Err(format!("Failed to set cache SetLock: {}", error).into()); + if self.cache.set(Arc::clone(&client.cache)).is_err() { + error!("Could not set cache OnceLock because it was already set. This should never happen."); + return Err("Could not set cache OnceLock because it was already set.".into()); } - if let Err(error) = self.data.set(Arc::clone(&client.data)) { - return Err(format!("Failed to set data SetLock: {}", error).into()); + if self.data.set(Arc::clone(&client.data)).is_err() { + error!("Could not set data OnceLock because it was already set. This should never happen."); + return Err("Could not set data OnceLock because it was already set.".into()); } - if let Err(error) = self.http.set(Arc::clone(&client.http)) { - return Err(format!("Failed to set http SetLock: {}", error).into()); + if self.http.set(Arc::clone(&client.http)).is_err() { + error!("Could not set http OnceLock because it was already set. This should never happen."); + return Err("Could not set http OnceLock because it was already set.".into()); } - if let Err(error) = self.shard_manager.set(Arc::clone(&client.shard_manager)) { - return Err(format!("Failed to set shard_manager SetLock: {}", error).into()); + if self.shard_manager.set(Arc::clone(&client.shard_manager)).is_err() { + error!("Could not set shard_manager OnceLock because it was already set. This should never happen."); + return Err("Could not set shard_manager OnceLock because it was already set.".into()); } if let Some(voice_manager) = &client.voice_manager { - if let Err(error) = self.voice_manager.set(Arc::clone(voice_manager)) { - return Err(format!("Failed to set voice_manager SetLock: {}", error).into()); + if self.voice_manager.set(Arc::clone(voice_manager)).is_err() { + error!("Could not set voice_manager OnceLock because it was already set. This should never happen."); + return Err("Could not set voice_manager OnceLock because it was already set.".into()); } + } else { + warn!("Voice manager is not available"); } - if let Err(error) = self.ws_url.set(Arc::clone(&client.ws_url)) { - return Err(format!("Failed to set ws_url SetLock: {}", error).into()); + if self.ws_url.set(Arc::clone(&client.ws_url)).is_err() { + error!("Could not set ws_url OnceLock because it was already set. This should never happen."); + return Err("Could not set ws_url OnceLock because it was already set.".into()); } let client_handle = spawn(async move { client.start().await }); @@ -138,12 +147,12 @@ impl Service for DiscordService { } struct EventHandler { - client: Arc>>, + client: Arc>, ready_notify: Arc, } impl EventHandler { - pub fn new(client: Arc>>, ready_notify: Arc) -> Self { + pub fn new(client: Arc>, ready_notify: Arc) -> Self { Self { client, ready_notify } } } @@ -152,9 +161,9 @@ impl EventHandler { impl client::EventHandler for EventHandler { async fn ready(&self, _ctx: Context, data_about_bot: Ready) { info!("Connected to Discord as {}", data_about_bot.user.tag()); - if let Err(error) = self.client.lock().await.set(data_about_bot) { - error!("Failed to set client SetLock: {}", error); - panic!("Failed to set client SetLock: {}", error); + if self.client.set(data_about_bot).is_err() { + error!("Could not set client OnceLock because it was already set. This should never happen."); + panic!("Could not set client OnceLock because it was already set"); } self.ready_notify.notify_one(); } diff --git a/src/service/service_manager.rs b/src/service/service_manager.rs index 78f4ecb..f715e5b 100644 --- a/src/service/service_manager.rs +++ b/src/service/service_manager.rs @@ -3,10 +3,10 @@ use super::{ types::{LifetimedPinnedBoxedFuture, OverallStatus, Priority, ShutdownError, StartupError, Status}, }; use crate::{ - event::EventRepeater, service::Watchdog, setlock::{SetLock, SetLockError} + event::EventRepeater, service::Watchdog }; use log::{error, info, warn}; -use std::{collections::HashMap, fmt::Display, mem, sync::{Arc, Weak}, time::Duration}; +use std::{collections::HashMap, fmt::Display, mem, sync::{Arc, OnceLock, Weak}, time::Duration}; use tokio::{ spawn, sync::{Mutex, MutexGuard}, @@ -54,7 +54,7 @@ pub fn new() -> Self { pub async fn build(self) -> Arc { let service_manager = ServiceManager { - weak: Mutex::new(SetLock::new()), + weak: OnceLock::new(), services: self.services, background_tasks: Mutex::new(HashMap::new()), on_status_change: EventRepeater::new("service_manager_on_status_change").await, @@ -63,14 +63,10 @@ pub fn new() -> Self { let arc = Arc::new(service_manager); let weak = Arc::downgrade(&arc); - let result = arc.weak.lock().await.set(weak); - if let Err(err) = result { - match err { - SetLockError::AlreadySet => { - error!("Unable to set ServiceManager's Weak self-reference in ServiceManagerBuilder because it was already set. This should never happen. Shutting down ungracefully to prevent further undefined behavior."); - unreachable!("Unable to set ServiceManager's Weak self-reference in ServiceManagerBuilder because it was already set."); - } - } + let result = arc.weak.set(weak); + if result.is_err() { + error!("Unable to set ServiceManager's Weak self-reference in ServiceManagerBuilder because it was already set. This should never happen. Shutting down ungracefully to prevent further undefined behavior."); + unreachable!("Unable to set ServiceManager's Weak self-reference in ServiceManagerBuilder because it was already set."); } arc @@ -78,7 +74,7 @@ pub fn new() -> Self { } pub struct ServiceManager { - weak: Mutex>>, + weak: OnceLock>, background_tasks: Mutex>>, pub services: Vec>>, @@ -326,8 +322,7 @@ impl ServiceManager { &self, service: &mut MutexGuard<'_, dyn Service>, ) -> Result<(), StartupError> { - let lock = self.weak.lock().await; - let weak = match lock.get() { + let weak = match self.weak.get() { Some(weak) => weak, None => { error!("ServiceManager's Weak self-reference was None while initializing service {}. This should never happen. Did you not use a ServiceManagerBuilder? Shutting down ungracefully to prevent further undefined behavior.", service.info().name); diff --git a/src/setlock.rs b/src/setlock.rs deleted file mode 100644 index 7bee17c..0000000 --- a/src/setlock.rs +++ /dev/null @@ -1,70 +0,0 @@ -use std::{error::Error, fmt::Display}; - -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] -pub enum SetLockError { - AlreadySet, -} - -impl Display for SetLockError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - SetLockError::AlreadySet => write!(f, "AlreadySet"), - } - } -} - -impl Error for SetLockError {} - -pub struct SetLock { - data: Option, -} - -impl SetLock { - pub fn new() -> Self { - Self { data: None } - } - - pub fn set(&mut self, data: T) -> Result<(), SetLockError> { - if self.data.is_some() { - return Err(SetLockError::AlreadySet); - } - - self.data = Some(data); - - Ok(()) - } - - pub fn is_set(&self) -> bool { - self.data.is_some() - } - - pub fn unwrap(&self) -> &T { - if self.data.is_none() { - panic!("unwrap called on an unset SetLock"); - } - - self.data.as_ref().unwrap() - } - - pub fn unwrap_mut(&mut self) -> &mut T { - if self.data.is_none() { - panic!("unwrap_mut called on an unset SetLock"); - } - - self.data.as_mut().unwrap() - } - - pub fn get(&self) -> Option<&T> { - self.data.as_ref() - } - - pub fn get_mut(&mut self) -> Option<&mut T> { - self.data.as_mut() - } -} - -impl Default for SetLock { - fn default() -> Self { - Self::new() - } -}