From fa4bee3bf7a8b6c6f7253520d0dc47d5c2cd6519 Mon Sep 17 00:00:00 2001 From: Lucas Meurer Date: Thu, 4 Sep 2025 10:31:26 +0200 Subject: [PATCH] Fix deadlock in `ImageLoader`, `FileLoader`, `EhttpLoader` (#7494) * Recently CI runs started to hang randomly: https://github.com/emilk/egui/actions/runs/17427449210/job/49477714447?pr=7359 This fixes the deadlock and adds the basic deadlock detection we also added to Mutexes in #7468. Also, interestingly, the more sophisticated deadlock detection (behind the deadlock_detection feature) didn't catch this for some reason. I wonder why it exists in the first place, when parking_lot also has built in deadlock detection? It also seems to make tests slower, widget_tests usually needs ~30s, with the deadlock detection removed its only ~12s. --- .../egui_extras/src/loaders/ehttp_loader.rs | 31 ++++++++++++------- crates/egui_extras/src/loaders/file_loader.rs | 23 +++++++++----- .../egui_extras/src/loaders/image_loader.rs | 27 +++++++++++----- crates/epaint/src/mutex.rs | 27 ++++++++++++++-- 4 files changed, 80 insertions(+), 28 deletions(-) diff --git a/crates/egui_extras/src/loaders/ehttp_loader.rs b/crates/egui_extras/src/loaders/ehttp_loader.rs index 7310cc6e..1d3e20c9 100644 --- a/crates/egui_extras/src/loaders/ehttp_loader.rs +++ b/crates/egui_extras/src/loaders/ehttp_loader.rs @@ -94,18 +94,27 @@ impl BytesLoader for EhttpLoader { Err(format!("Failed to load {uri:?}")) } }; - let mut cache = cache.lock(); - if let std::collections::hash_map::Entry::Occupied(mut entry) = - cache.entry(uri.clone()) - { - let entry = entry.get_mut(); - *entry = Poll::Ready(result); + let repaint = { + let mut cache = cache.lock(); + if let std::collections::hash_map::Entry::Occupied(mut entry) = + cache.entry(uri.clone()) + { + let entry = entry.get_mut(); + *entry = Poll::Ready(result); + ctx.request_repaint(); + log::trace!("Finished loading {uri:?}"); + true + } else { + log::trace!( + "Canceled loading {uri:?}\nNote: This can happen if `forget_image` is called while the image is still loading." + ); + false + } + }; + // We may not lock Context while the cache lock is held (see ImageLoader::load + // for details). + if repaint { ctx.request_repaint(); - log::trace!("Finished loading {uri:?}"); - } else { - log::trace!( - "Canceled loading {uri:?}\nNote: This can happen if `forget_image` is called while the image is still loading." - ); } } }); diff --git a/crates/egui_extras/src/loaders/file_loader.rs b/crates/egui_extras/src/loaders/file_loader.rs index 001e988c..5e66b76d 100644 --- a/crates/egui_extras/src/loaders/file_loader.rs +++ b/crates/egui_extras/src/loaders/file_loader.rs @@ -95,14 +95,23 @@ impl BytesLoader for FileLoader { } Err(err) => Err(err.to_string()), }; - let mut cache = cache.lock(); - if let std::collections::hash_map::Entry::Occupied(mut entry) = cache.entry(uri.clone()) { - let entry = entry.get_mut(); - *entry = Poll::Ready(result); + let repaint = { + let mut cache = cache.lock(); + if let std::collections::hash_map::Entry::Occupied(mut entry) = cache.entry(uri.clone()) { + let entry = entry.get_mut(); + *entry = Poll::Ready(result); + ctx.request_repaint(); + log::trace!("Finished loading {uri:?}"); + true + } else { + log::trace!("Canceled loading {uri:?}\nNote: This can happen if `forget_image` is called while the image is still loading."); + false + } + }; + // We may not lock Context while the cache lock is held (see ImageLoader::load + // for details). + if repaint { ctx.request_repaint(); - log::trace!("Finished loading {uri:?}"); - } else { - log::trace!("Canceled loading {uri:?}\nNote: This can happen if `forget_image` is called while the image is still loading."); } } }) diff --git a/crates/egui_extras/src/loaders/image_loader.rs b/crates/egui_extras/src/loaders/image_loader.rs index 82df87b3..3acb6b58 100644 --- a/crates/egui_extras/src/loaders/image_loader.rs +++ b/crates/egui_extras/src/loaders/image_loader.rs @@ -100,15 +100,28 @@ impl ImageLoader for ImageCrateLoader { let result = crate::image::load_image_bytes(&bytes) .map(Arc::new) .map_err(|err| err.to_string()); - let mut cache = cache.lock(); + let repaint = { + let mut cache = cache.lock(); - if let std::collections::hash_map::Entry::Occupied(mut entry) = cache.entry(uri.clone()) { - let entry = entry.get_mut(); - *entry = Poll::Ready(result); + if let std::collections::hash_map::Entry::Occupied(mut entry) = cache.entry(uri.clone()) { + let entry = entry.get_mut(); + *entry = Poll::Ready(result); + log::trace!("ImageLoader - finished loading {uri:?}"); + true + } else { + log::trace!("ImageLoader - canceled loading {uri:?}\nNote: This can happen if `forget_image` is called while the image is still loading."); + false + } + }; + // We may not lock Context while the cache lock is held, since this can + // deadlock. + // Example deadlock scenario: + // - loader thread: lock cache + // - main thread: lock ctx (e.g. in `Context::has_pending_images`) + // - loader thread: try to lock ctx (in `request_repaint`) + // - main thread: try to lock cache (from `Self::has_pending`) + if repaint { ctx.request_repaint(); - log::trace!("ImageLoader - finished loading {uri:?}"); - } else { - log::trace!("ImageLoader - canceled loading {uri:?}\nNote: This can happen if `forget_image` is called while the image is still loading."); } } }) diff --git a/crates/epaint/src/mutex.rs b/crates/epaint/src/mutex.rs index ef1b3a6c..1a043b37 100644 --- a/crates/epaint/src/mutex.rs +++ b/crates/epaint/src/mutex.rs @@ -2,8 +2,13 @@ // ---------------------------------------------------------------------------- +#[cfg(not(feature = "deadlock_detection"))] +const DEADLOCK_DURATION: std::time::Duration = std::time::Duration::from_secs(30); + #[cfg(not(feature = "deadlock_detection"))] mod mutex_impl { + use super::DEADLOCK_DURATION; + /// Provides interior mutability. /// /// This is a thin wrapper around [`parking_lot::Mutex`], except if @@ -25,7 +30,7 @@ mod mutex_impl { pub fn lock(&self) -> MutexGuard<'_, T> { if cfg!(debug_assertions) { self.0 - .try_lock_for(std::time::Duration::from_secs(30)) + .try_lock_for(DEADLOCK_DURATION) .expect("Looks like a deadlock!") } else { self.0.lock() @@ -127,6 +132,8 @@ mod mutex_impl { #[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; @@ -151,12 +158,26 @@ mod rw_lock_impl { impl RwLock { #[inline(always)] pub fn read(&self) -> RwLockReadGuard<'_, T> { - parking_lot::RwLockReadGuard::map(self.0.read(), |v| v) + 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> { - parking_lot::RwLockWriteGuard::map(self.0.write(), |v| v) + 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) } } }