From 30a8c06fd9caa4276a4261a107fc84414e36ce6c Mon Sep 17 00:00:00 2001 From: Jade Ellis Date: Sat, 19 Jul 2025 20:36:27 +0100 Subject: [PATCH] refactor: Replace std Mutex with parking_lot --- Cargo.lock | 1 + Cargo.toml | 7 +++++++ src/admin/processor.rs | 16 +++++----------- src/core/Cargo.toml | 1 + src/core/info/rustc.rs | 10 +++------- src/core/log/capture/layer.rs | 2 +- src/core/log/capture/mod.rs | 8 +++++--- src/core/log/capture/util.rs | 12 ++++++------ src/core/log/reload.rs | 16 ++++------------ src/core/mod.rs | 1 + src/core/utils/mutex_map.rs | 25 ++++++++----------------- src/macros/rustc.rs | 4 ++-- 12 files changed, 44 insertions(+), 59 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6f711007..b084f72a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -967,6 +967,7 @@ dependencies = [ "maplit", "nix", "num-traits", + "parking_lot", "rand 0.8.5", "regex", "reqwest", diff --git a/Cargo.toml b/Cargo.toml index ef917332..3e52c4b2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -515,6 +515,13 @@ version = "1.0" [workspace.dependencies.proc-macro2] version = "1.0" +[workspace.dependencies.parking_lot] +version = "0.12.4" + +# Use this when extending with_lock::WithLock to parking_lot +# [workspace.dependencies.lock_api] +# version = "0.4.13" + [workspace.dependencies.bytesize] version = "2.0" diff --git a/src/admin/processor.rs b/src/admin/processor.rs index e80000c1..2c91efe1 100644 --- a/src/admin/processor.rs +++ b/src/admin/processor.rs @@ -1,14 +1,8 @@ -use std::{ - fmt::Write, - mem::take, - panic::AssertUnwindSafe, - sync::{Arc, Mutex}, - time::SystemTime, -}; +use std::{fmt::Write, mem::take, panic::AssertUnwindSafe, sync::Arc, time::SystemTime}; use clap::{CommandFactory, Parser}; use conduwuit::{ - Error, Result, debug, error, + Error, Result, SyncMutex, debug, error, log::{ capture, capture::Capture, @@ -123,7 +117,7 @@ async fn process( let mut output = String::new(); // Prepend the logs only if any were captured - let logs = logs.lock().expect("locked"); + let logs = logs.lock(); if logs.lines().count() > 2 { writeln!(&mut output, "{logs}").expect("failed to format logs to command output"); } @@ -132,7 +126,7 @@ async fn process( (result, output) } -fn capture_create(context: &Context<'_>) -> (Arc, Arc>) { +fn capture_create(context: &Context<'_>) -> (Arc, Arc>) { let env_config = &context.services.server.config.admin_log_capture; let env_filter = EnvFilter::try_new(env_config).unwrap_or_else(|e| { warn!("admin_log_capture filter invalid: {e:?}"); @@ -152,7 +146,7 @@ fn capture_create(context: &Context<'_>) -> (Arc, Arc>) { data.level() <= log_level && data.our_modules() && data.scope.contains(&"admin") }; - let logs = Arc::new(Mutex::new( + let logs = Arc::new(SyncMutex::new( collect_stream(|s| markdown_table_head(s)).expect("markdown table header"), )); diff --git a/src/core/Cargo.toml b/src/core/Cargo.toml index 0c33c590..7a3721d6 100644 --- a/src/core/Cargo.toml +++ b/src/core/Cargo.toml @@ -110,6 +110,7 @@ tracing-core.workspace = true tracing-subscriber.workspace = true tracing.workspace = true url.workspace = true +parking_lot.workspace = true [target.'cfg(unix)'.dependencies] nix.workspace = true diff --git a/src/core/info/rustc.rs b/src/core/info/rustc.rs index 048c0cd5..60156301 100644 --- a/src/core/info/rustc.rs +++ b/src/core/info/rustc.rs @@ -3,18 +3,15 @@ //! several crates, lower-level information is supplied from each crate during //! static initialization. -use std::{ - collections::BTreeMap, - sync::{Mutex, OnceLock}, -}; +use std::{collections::BTreeMap, sync::OnceLock}; -use crate::utils::exchange; +use crate::{SyncMutex, utils::exchange}; /// Raw capture of rustc flags used to build each crate in the project. Informed /// by rustc_flags_capture macro (one in each crate's mod.rs). This is /// done during static initialization which is why it's mutex-protected and pub. /// Should not be written to by anything other than our macro. -pub static FLAGS: Mutex> = Mutex::new(BTreeMap::new()); +pub static FLAGS: SyncMutex> = SyncMutex::new(BTreeMap::new()); /// Processed list of enabled features across all project crates. This is /// generated from the data in FLAGS. @@ -27,7 +24,6 @@ fn init_features() -> Vec<&'static str> { let mut features = Vec::new(); FLAGS .lock() - .expect("locked") .iter() .for_each(|(_, flags)| append_features(&mut features, flags)); diff --git a/src/core/log/capture/layer.rs b/src/core/log/capture/layer.rs index 381a652f..b3235d91 100644 --- a/src/core/log/capture/layer.rs +++ b/src/core/log/capture/layer.rs @@ -55,7 +55,7 @@ where let mut visitor = Visitor { values: Values::new() }; event.record(&mut visitor); - let mut closure = capture.closure.lock().expect("exclusive lock"); + let mut closure = capture.closure.lock(); closure(Data { layer, event, diff --git a/src/core/log/capture/mod.rs b/src/core/log/capture/mod.rs index 20f70091..b7e5d2b5 100644 --- a/src/core/log/capture/mod.rs +++ b/src/core/log/capture/mod.rs @@ -4,7 +4,7 @@ pub mod layer; pub mod state; pub mod util; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; pub use data::Data; use guard::Guard; @@ -12,6 +12,8 @@ pub use layer::{Layer, Value}; pub use state::State; pub use util::*; +use crate::SyncMutex; + pub type Filter = dyn Fn(Data<'_>) -> bool + Send + Sync + 'static; pub type Closure = dyn FnMut(Data<'_>) + Send + Sync + 'static; @@ -19,7 +21,7 @@ pub type Closure = dyn FnMut(Data<'_>) + Send + Sync + 'static; pub struct Capture { state: Arc, filter: Option>, - closure: Mutex>, + closure: SyncMutex>, } impl Capture { @@ -34,7 +36,7 @@ impl Capture { Arc::new(Self { state: state.clone(), filter: filter.map(|p| -> Box { Box::new(p) }), - closure: Mutex::new(Box::new(closure)), + closure: SyncMutex::new(Box::new(closure)), }) } diff --git a/src/core/log/capture/util.rs b/src/core/log/capture/util.rs index 65524be5..21a416a9 100644 --- a/src/core/log/capture/util.rs +++ b/src/core/log/capture/util.rs @@ -1,31 +1,31 @@ -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use super::{ super::{Level, fmt}, Closure, Data, }; -use crate::Result; +use crate::{Result, SyncMutex}; -pub fn fmt_html(out: Arc>) -> Box +pub fn fmt_html(out: Arc>) -> Box where S: std::fmt::Write + Send + 'static, { fmt(fmt::html, out) } -pub fn fmt_markdown(out: Arc>) -> Box +pub fn fmt_markdown(out: Arc>) -> Box where S: std::fmt::Write + Send + 'static, { fmt(fmt::markdown, out) } -pub fn fmt(fun: F, out: Arc>) -> Box +pub fn fmt(fun: F, out: Arc>) -> Box where F: Fn(&mut S, &Level, &str, &str) -> Result<()> + Send + Sync + Copy + 'static, S: std::fmt::Write + Send + 'static, { - Box::new(move |data| call(fun, &mut *out.lock().expect("locked"), &data)) + Box::new(move |data| call(fun, &mut *out.lock(), &data)) } fn call(fun: F, out: &mut S, data: &Data<'_>) diff --git a/src/core/log/reload.rs b/src/core/log/reload.rs index f72fde47..356ee9f2 100644 --- a/src/core/log/reload.rs +++ b/src/core/log/reload.rs @@ -1,11 +1,8 @@ -use std::{ - collections::HashMap, - sync::{Arc, Mutex}, -}; +use std::{collections::HashMap, sync::Arc}; use tracing_subscriber::{EnvFilter, reload}; -use crate::{Result, error}; +use crate::{Result, SyncMutex, error}; /// We need to store a reload::Handle value, but can't name it's type explicitly /// because the S type parameter depends on the subscriber's previous layers. In @@ -35,7 +32,7 @@ impl ReloadHandle for reload::Handle { #[derive(Clone)] pub struct LogLevelReloadHandles { - handles: Arc>, + handles: Arc>, } type HandleMap = HashMap; @@ -43,16 +40,12 @@ type Handle = Box + Send + Sync>; impl LogLevelReloadHandles { pub fn add(&self, name: &str, handle: Handle) { - self.handles - .lock() - .expect("locked") - .insert(name.into(), handle); + self.handles.lock().insert(name.into(), handle); } pub fn reload(&self, new_value: &EnvFilter, names: Option<&[&str]>) -> Result<()> { self.handles .lock() - .expect("locked") .iter() .filter(|(name, _)| names.is_some_and(|names| names.contains(&name.as_str()))) .for_each(|(_, handle)| { @@ -66,7 +59,6 @@ impl LogLevelReloadHandles { pub fn current(&self, name: &str) -> Option { self.handles .lock() - .expect("locked") .get(name) .map(|handle| handle.current())? } diff --git a/src/core/mod.rs b/src/core/mod.rs index d99139be..363fece8 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -28,6 +28,7 @@ pub use info::{ pub use matrix::{ Event, EventTypeExt, Pdu, PduCount, PduEvent, PduId, RoomVersion, pdu, state_res, }; +pub use parking_lot::{Mutex as SyncMutex, RwLock as SyncRwLock}; pub use server::Server; pub use utils::{ctor, dtor, implement, result, result::Result}; diff --git a/src/core/utils/mutex_map.rs b/src/core/utils/mutex_map.rs index 01504ce6..ddb361a4 100644 --- a/src/core/utils/mutex_map.rs +++ b/src/core/utils/mutex_map.rs @@ -1,12 +1,8 @@ -use std::{ - fmt::Debug, - hash::Hash, - sync::{Arc, TryLockError::WouldBlock}, -}; +use std::{fmt::Debug, hash::Hash, sync::Arc}; use tokio::sync::OwnedMutexGuard as Omg; -use crate::{Result, err}; +use crate::{Result, SyncMutex, err}; /// Map of Mutexes pub struct MutexMap { @@ -19,7 +15,7 @@ pub struct Guard { } type Map = Arc>; -type MapMutex = std::sync::Mutex>; +type MapMutex = SyncMutex>; type HashMap = std::collections::HashMap>; type Value = Arc>; @@ -45,7 +41,6 @@ where let val = self .map .lock() - .expect("locked") .entry(k.try_into().expect("failed to construct key")) .or_default() .clone(); @@ -66,7 +61,6 @@ where let val = self .map .lock() - .expect("locked") .entry(k.try_into().expect("failed to construct key")) .or_default() .clone(); @@ -87,10 +81,7 @@ where let val = self .map .try_lock() - .map_err(|e| match e { - | WouldBlock => err!("would block"), - | _ => panic!("{e:?}"), - })? + .ok_or_else(|| err!("would block"))? .entry(k.try_into().expect("failed to construct key")) .or_default() .clone(); @@ -102,13 +93,13 @@ where } #[must_use] - pub fn contains(&self, k: &Key) -> bool { self.map.lock().expect("locked").contains_key(k) } + pub fn contains(&self, k: &Key) -> bool { self.map.lock().contains_key(k) } #[must_use] - pub fn is_empty(&self) -> bool { self.map.lock().expect("locked").is_empty() } + pub fn is_empty(&self) -> bool { self.map.lock().is_empty() } #[must_use] - pub fn len(&self) -> usize { self.map.lock().expect("locked").len() } + pub fn len(&self) -> usize { self.map.lock().len() } } impl Default for MutexMap @@ -123,7 +114,7 @@ impl Drop for Guard { #[tracing::instrument(name = "unlock", level = "trace", skip_all)] fn drop(&mut self) { if Arc::strong_count(Omg::mutex(&self.val)) <= 2 { - self.map.lock().expect("locked").retain(|_, val| { + self.map.lock().retain(|_, val| { !Arc::ptr_eq(val, Omg::mutex(&self.val)) || Arc::strong_count(val) > 2 }); } diff --git a/src/macros/rustc.rs b/src/macros/rustc.rs index 1220c8d4..cf935fe5 100644 --- a/src/macros/rustc.rs +++ b/src/macros/rustc.rs @@ -15,13 +15,13 @@ pub(super) fn flags_capture(args: TokenStream) -> TokenStream { #[conduwuit_core::ctor] fn _set_rustc_flags() { - conduwuit_core::info::rustc::FLAGS.lock().expect("locked").insert(#crate_name, &RUSTC_FLAGS); + conduwuit_core::info::rustc::FLAGS.lock().insert(#crate_name, &RUSTC_FLAGS); } // static strings have to be yanked on module unload #[conduwuit_core::dtor] fn _unset_rustc_flags() { - conduwuit_core::info::rustc::FLAGS.lock().expect("locked").remove(#crate_name); + conduwuit_core::info::rustc::FLAGS.lock().remove(#crate_name); } };