From 80d61a7c534131b10a5d6c43d408b7f7afb62633 Mon Sep 17 00:00:00 2001 From: Lucas Meurer Date: Thu, 4 Sep 2025 12:57:09 +0200 Subject: [PATCH] Remove the `deadlock_detection` feature (#7497) * related #7494 Removes the `deadlock_detection` feature, since we now have a more primitive panic-after-30s deadlock detection which works well enough and even detects kinds of deadlocks that the `deadlock_detection` feature never supported. --- Cargo.lock | 1 - crates/egui/Cargo.toml | 5 - crates/egui_demo_app/tests/test_demo_app.rs | 3 + crates/epaint/Cargo.toml | 10 - crates/epaint/src/mutex.rs | 470 ++++---------------- 5 files changed, 94 insertions(+), 395 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7497726f..e799591d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1535,7 +1535,6 @@ version = "0.32.1" dependencies = [ "ab_glyph", "ahash", - "backtrace", "bytemuck", "criterion", "document-features", diff --git a/crates/egui/Cargo.toml b/crates/egui/Cargo.toml index 238a8d8e..32951c05 100644 --- a/crates/egui/Cargo.toml +++ b/crates/egui/Cargo.toml @@ -44,11 +44,6 @@ cint = ["epaint/cint"] ## Enable the [`hex_color`] macro. color-hex = ["epaint/color-hex"] -## This will automatically detect deadlocks due to double-locking on the same thread. -## If your app freezes, you may want to enable this! -## Only affects [`epaint::mutex::RwLock`] (which egui uses a lot). -deadlock_detection = ["epaint/deadlock_detection"] - ## If set, egui will use `include_bytes!` to bundle some fonts. ## If you plan on specifying your own fonts you may disable this feature. default_fonts = ["epaint/default_fonts"] diff --git a/crates/egui_demo_app/tests/test_demo_app.rs b/crates/egui_demo_app/tests/test_demo_app.rs index 63b3b2a8..e083c845 100644 --- a/crates/egui_demo_app/tests/test_demo_app.rs +++ b/crates/egui_demo_app/tests/test_demo_app.rs @@ -62,6 +62,9 @@ fn test_demo_app() { .type_text("file://../eframe/data/icon.png"); harness.get_by_role_and_label(Role::Button, "✔").click(); + + // Wait for the image to load + harness.try_run_realtime().ok(); } _ => {} } diff --git a/crates/epaint/Cargo.toml b/crates/epaint/Cargo.toml index 5a401698..3cbb4f47 100644 --- a/crates/epaint/Cargo.toml +++ b/crates/epaint/Cargo.toml @@ -40,11 +40,6 @@ cint = ["ecolor/cint"] ## Enable the [`hex_color`] macro. color-hex = ["ecolor/color-hex"] -## This will automatically detect deadlocks due to double-locking on the same thread. -## If your app freezes, you may want to enable this! -## Only affects [`mutex::RwLock`] (which epaint and egui uses a lot). -deadlock_detection = ["dep:backtrace"] - ## If set, epaint will use `include_bytes!` to bundle some fonts. ## If you plan on specifying your own fonts you may disable this feature. default_fonts = ["epaint_default_fonts"] @@ -94,11 +89,6 @@ serde = { workspace = true, optional = true, features = ["derive", "rc"] } epaint_default_fonts = { workspace = true, optional = true } -# native: -[target.'cfg(not(target_arch = "wasm32"))'.dependencies] -backtrace = { workspace = true, optional = true } - - [dev-dependencies] criterion.workspace = true mimalloc.workspace = true diff --git a/crates/epaint/src/mutex.rs b/crates/epaint/src/mutex.rs index 1a043b37..4b6a66f9 100644 --- a/crates/epaint/src/mutex.rs +++ b/crates/epaint/src/mutex.rs @@ -1,394 +1,107 @@ -//! Helper module that adds extra checks when the `deadlock_detection` feature is turned on. +//! Wrappers around `parking_lot` locks, with a simple deadlock detection mechanism. // ---------------------------------------------------------------------------- -#[cfg(not(feature = "deadlock_detection"))] -const DEADLOCK_DURATION: std::time::Duration = std::time::Duration::from_secs(30); +const DEADLOCK_DURATION: std::time::Duration = std::time::Duration::from_secs(10); -#[cfg(not(feature = "deadlock_detection"))] -mod mutex_impl { - use super::DEADLOCK_DURATION; +/// Provides interior mutability. +/// +/// It's tailored for internal use in egui should only be used for short locks (as a guideline, +/// locks should never be held longer than a single frame). In debug builds, when a lock can't +/// be acquired within 10 seconds, we assume a deadlock and will panic. +/// +/// This is a thin wrapper around [`parking_lot::Mutex`]. +#[derive(Default)] +pub struct Mutex(parking_lot::Mutex); - /// Provides interior mutability. +/// The lock you get from [`Mutex`]. +pub use parking_lot::MutexGuard; + +impl Mutex { + #[inline(always)] + pub fn new(val: T) -> Self { + Self(parking_lot::Mutex::new(val)) + } + + /// Try to acquire the lock. /// - /// This is a thin wrapper around [`parking_lot::Mutex`], except if - /// the feature `deadlock_detection` is turned enabled, in which case - /// extra checks are added to detect deadlocks. - #[derive(Default)] - pub struct Mutex(parking_lot::Mutex); - - /// The lock you get from [`Mutex`]. - pub use parking_lot::MutexGuard; - - impl Mutex { - #[inline(always)] - pub fn new(val: T) -> Self { - Self(parking_lot::Mutex::new(val)) - } - - #[inline(always)] - pub fn lock(&self) -> MutexGuard<'_, T> { - if cfg!(debug_assertions) { - self.0 - .try_lock_for(DEADLOCK_DURATION) - .expect("Looks like a deadlock!") - } else { - self.0.lock() - } - } - } -} - -#[cfg(feature = "deadlock_detection")] -mod mutex_impl { - /// Provides interior mutability. - /// - /// This is a thin wrapper around [`parking_lot::Mutex`], except if - /// the feature `deadlock_detection` is turned enabled, in which case - /// extra checks are added to detect deadlocks. - #[derive(Default)] - pub struct Mutex(parking_lot::Mutex); - - /// The lock you get from [`Mutex`]. - pub struct MutexGuard<'a, T>(parking_lot::MutexGuard<'a, T>, *const ()); - - #[derive(Default)] - struct HeldLocks(Vec<*const ()>); - - impl HeldLocks { - #[inline(always)] - fn insert(&mut self, lock: *const ()) { - // Very few locks will ever be held at the same time, so a linear search is fast - assert!( - !self.0.contains(&lock), - "Recursively locking a Mutex in the same thread is not supported" - ); - self.0.push(lock); - } - - #[inline(always)] - fn remove(&mut self, lock: *const ()) { - self.0.retain(|&ptr| ptr != lock); - } - } - - thread_local! { - static HELD_LOCKS_TLS: std::cell::RefCell = Default::default(); - } - - impl Mutex { - #[inline(always)] - pub fn new(val: T) -> Self { - Self(parking_lot::Mutex::new(val)) - } - - pub fn lock(&self) -> MutexGuard<'_, T> { - // Detect if we are recursively taking out a lock on this mutex. - - // use a pointer to the inner data as an id for this lock - let ptr = std::ptr::from_ref::>(&self.0).cast::<()>(); - - // Store it in thread local storage while we have a lock guard taken out - HELD_LOCKS_TLS.with(|held_locks| { - held_locks.borrow_mut().insert(ptr); - }); - - MutexGuard(self.0.lock(), ptr) - } - - #[inline(always)] - pub fn into_inner(self) -> T { - self.0.into_inner() - } - } - - impl Drop for MutexGuard<'_, T> { - fn drop(&mut self) { - let ptr = self.1; - HELD_LOCKS_TLS.with(|held_locks| { - held_locks.borrow_mut().remove(ptr); - }); - } - } - - impl std::ops::Deref for MutexGuard<'_, T> { - type Target = T; - - #[inline(always)] - fn deref(&self) -> &Self::Target { - &self.0 - } - } - - impl std::ops::DerefMut for MutexGuard<'_, T> { - #[inline(always)] - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 - } - } -} - -// ---------------------------------------------------------------------------- - -#[cfg(not(feature = "deadlock_detection"))] -mod rw_lock_impl { - use super::DEADLOCK_DURATION; - - /// The lock you get from [`RwLock::read`]. - pub use parking_lot::MappedRwLockReadGuard as RwLockReadGuard; - - /// The lock you get from [`RwLock::write`]. - pub use parking_lot::MappedRwLockWriteGuard as RwLockWriteGuard; - - /// Provides interior mutability. - /// - /// This is a thin wrapper around [`parking_lot::RwLock`], except if - /// the feature `deadlock_detection` is turned enabled, in which case - /// extra checks are added to detect deadlocks. - #[derive(Default)] - pub struct RwLock(parking_lot::RwLock); - - impl RwLock { - #[inline(always)] - pub fn new(val: T) -> Self { - Self(parking_lot::RwLock::new(val)) - } - } - - impl RwLock { - #[inline(always)] - pub fn read(&self) -> RwLockReadGuard<'_, T> { - let guard = if cfg!(debug_assertions) { - self.0 - .try_read_for(DEADLOCK_DURATION) - .expect("Looks like a deadlock!") - } else { - self.0.read() - }; - parking_lot::RwLockReadGuard::map(guard, |v| v) - } - - #[inline(always)] - pub fn write(&self) -> RwLockWriteGuard<'_, T> { - let guard = if cfg!(debug_assertions) { - self.0 - .try_write_for(DEADLOCK_DURATION) - .expect("Looks like a deadlock!") - } else { - self.0.write() - }; - parking_lot::RwLockWriteGuard::map(guard, |v| v) - } - } -} - -#[cfg(feature = "deadlock_detection")] -mod rw_lock_impl { - use std::{ - ops::{Deref, DerefMut}, - sync::Arc, - thread::ThreadId, - }; - - use ahash::HashMap; - use parking_lot::{MappedRwLockReadGuard, MappedRwLockWriteGuard}; - - /// The lock you get from [`RwLock::read`]. - pub struct RwLockReadGuard<'a, T> { - // The option is used only because we need to `take()` the guard out of self - // when doing remappings (`map()`), i.e. it's used as a safe `ManuallyDrop`. - guard: Option>, - holders: Arc>>, - } - - impl<'a, T> RwLockReadGuard<'a, T> { - #[inline] - pub fn map(mut s: Self, f: F) -> RwLockReadGuard<'a, U> - where - F: FnOnce(&T) -> &U, - { - RwLockReadGuard { - guard: s - .guard - .take() - .map(|g| parking_lot::MappedRwLockReadGuard::map(g, f)), - holders: Arc::clone(&s.holders), - } - } - } - - impl Deref for RwLockReadGuard<'_, T> { - type Target = T; - - fn deref(&self) -> &Self::Target { - self.guard.as_ref().unwrap() - } - } - - impl Drop for RwLockReadGuard<'_, T> { - fn drop(&mut self) { - let tid = std::thread::current().id(); - self.holders.lock().remove(&tid); - } - } - - /// The lock you get from [`RwLock::write`]. - pub struct RwLockWriteGuard<'a, T> { - // The option is used only because we need to `take()` the guard out of self - // when doing remappings (`map()`), i.e. it's used as a safe `ManuallyDrop`. - guard: Option>, - holders: Arc>>, - } - - impl<'a, T> RwLockWriteGuard<'a, T> { - #[inline] - pub fn map(mut s: Self, f: F) -> RwLockWriteGuard<'a, U> - where - F: FnOnce(&mut T) -> &mut U, - { - RwLockWriteGuard { - guard: s - .guard - .take() - .map(|g| parking_lot::MappedRwLockWriteGuard::map(g, f)), - holders: Arc::clone(&s.holders), - } - } - } - - impl Deref for RwLockWriteGuard<'_, T> { - type Target = T; - - fn deref(&self) -> &Self::Target { - self.guard.as_ref().unwrap() - } - } - - impl DerefMut for RwLockWriteGuard<'_, T> { - fn deref_mut(&mut self) -> &mut Self::Target { - self.guard.as_mut().unwrap() - } - } - - impl Drop for RwLockWriteGuard<'_, T> { - fn drop(&mut self) { - let tid = std::thread::current().id(); - self.holders.lock().remove(&tid); - } - } - - /// Provides interior mutability. - /// - /// This is a thin wrapper around [`parking_lot::RwLock`], except if - /// the feature `deadlock_detection` is turned enabled, in which case - /// extra checks are added to detect deadlocks. - #[derive(Default)] - pub struct RwLock { - lock: parking_lot::RwLock, - // Technically we'd need a list of backtraces per thread-id since parking_lot's - // read-locks are reentrant. - // In practice it's not that useful to have the whole list though, so we only - // keep track of the first backtrace for now. - holders: Arc>>, - } - - impl RwLock { - pub fn new(val: T) -> Self { - Self { - lock: parking_lot::RwLock::new(val), - holders: Default::default(), - } - } - - pub fn read(&self) -> RwLockReadGuard<'_, T> { - let tid = std::thread::current().id(); - - // If it is write-locked, and we locked it (reentrancy deadlock) - let would_deadlock = - self.lock.is_locked_exclusive() && self.holders.lock().contains_key(&tid); - assert!( - !would_deadlock, - "{} DEAD-LOCK DETECTED ({:?})!\n\ - Trying to grab read-lock at:\n{}\n\ - which is already exclusively held by current thread at:\n{}\n\n", - std::any::type_name::(), - tid, - format_backtrace(&mut make_backtrace()), - format_backtrace(self.holders.lock().get_mut(&tid).unwrap()) - ); - - self.holders - .lock() - .entry(tid) - .or_insert_with(make_backtrace); - - RwLockReadGuard { - guard: parking_lot::RwLockReadGuard::map(self.lock.read(), |v| v).into(), - holders: Arc::clone(&self.holders), - } - } - - pub fn write(&self) -> RwLockWriteGuard<'_, T> { - let tid = std::thread::current().id(); - - // If it is locked in any way, and we locked it (reentrancy deadlock) - let would_deadlock = self.lock.is_locked() && self.holders.lock().contains_key(&tid); - assert!( - !would_deadlock, - "{} DEAD-LOCK DETECTED ({:?})!\n\ - Trying to grab write-lock at:\n{}\n\ - which is already held by current thread at:\n{}\n\n", - std::any::type_name::(), - tid, - format_backtrace(&mut make_backtrace()), - format_backtrace(self.holders.lock().get_mut(&tid).unwrap()) - ); - - self.holders - .lock() - .entry(tid) - .or_insert_with(make_backtrace); - - RwLockWriteGuard { - guard: parking_lot::RwLockWriteGuard::map(self.lock.write(), |v| v).into(), - holders: Arc::clone(&self.holders), - } - } - - #[inline(always)] - pub fn into_inner(self) -> T { - self.lock.into_inner() - } - } - - fn make_backtrace() -> backtrace::Backtrace { - backtrace::Backtrace::new_unresolved() - } - - fn format_backtrace(backtrace: &mut backtrace::Backtrace) -> String { - backtrace.resolve(); - - let stacktrace = format!("{backtrace:?}"); - - // Remove irrelevant parts of the stacktrace: - let end_offset = stacktrace - .find("std::sys_common::backtrace::__rust_begin_short_backtrace") - .unwrap_or(stacktrace.len()); - let stacktrace = &stacktrace[..end_offset]; - - let first_interesting_function = "epaint::mutex::rw_lock_impl::make_backtrace\n"; - if let Some(start_offset) = stacktrace.find(first_interesting_function) { - stacktrace[start_offset + first_interesting_function.len()..].to_owned() + /// ## Panics + /// Will panic in debug builds if the lock can't be acquired within 10 seconds. + #[inline(always)] + #[cfg_attr(debug_assertions, track_caller)] + pub fn lock(&self) -> MutexGuard<'_, T> { + if cfg!(debug_assertions) { + self.0 + .try_lock_for(DEADLOCK_DURATION) + .expect("Looks like a deadlock!") } else { - stacktrace.to_owned() + self.0.lock() } } } // ---------------------------------------------------------------------------- -pub use mutex_impl::{Mutex, MutexGuard}; -pub use rw_lock_impl::{RwLock, RwLockReadGuard, RwLockWriteGuard}; +/// The lock you get from [`RwLock::read`]. +pub use parking_lot::MappedRwLockReadGuard as RwLockReadGuard; + +/// The lock you get from [`RwLock::write`]. +pub use parking_lot::MappedRwLockWriteGuard as RwLockWriteGuard; + +/// Provides interior mutability. +/// +/// It's tailored for internal use in egui should only be used for short locks (as a guideline, +/// locks should never be held longer than a single frame). In debug builds, when a lock can't +/// be acquired within 10 seconds, we assume a deadlock and will panic. +/// +/// This is a thin wrapper around [`parking_lot::RwLock`]. +#[derive(Default)] +pub struct RwLock(parking_lot::RwLock); + +impl RwLock { + #[inline(always)] + pub fn new(val: T) -> Self { + Self(parking_lot::RwLock::new(val)) + } +} + +impl RwLock { + /// Try to acquire read-access to the lock. + /// + /// ## Panics + /// Will panic in debug builds if the lock can't be acquired within 10 seconds. + #[inline(always)] + #[cfg_attr(debug_assertions, track_caller)] + pub fn read(&self) -> RwLockReadGuard<'_, T> { + let guard = if cfg!(debug_assertions) { + self.0 + .try_read_for(DEADLOCK_DURATION) + .expect("Looks like a deadlock!") + } else { + self.0.read() + }; + parking_lot::RwLockReadGuard::map(guard, |v| v) + } + + /// Try to acquire write-access to the lock. + /// + /// ## Panics + /// Will panic in debug builds if the lock can't be acquired within 10 seconds. + #[inline(always)] + #[cfg_attr(debug_assertions, track_caller)] + pub fn write(&self) -> RwLockWriteGuard<'_, T> { + let guard = if cfg!(debug_assertions) { + self.0 + .try_write_for(DEADLOCK_DURATION) + .expect("Looks like a deadlock!") + } else { + self.0.write() + }; + parking_lot::RwLockWriteGuard::map(guard, |v| v) + } +} + +// ---------------------------------------------------------------------------- impl Clone for Mutex where @@ -434,7 +147,6 @@ mod tests { } #[cfg(not(target_arch = "wasm32"))] -#[cfg(feature = "deadlock_detection")] #[cfg(test)] mod tests_rwlock { #![allow(clippy::disallowed_methods)] // Ok for tests