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.
This commit is contained in:
Lucas Meurer 2025-09-04 10:31:26 +02:00 committed by GitHub
parent d3cd6d44cf
commit fa4bee3bf7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 80 additions and 28 deletions

View File

@ -94,18 +94,27 @@ impl BytesLoader for EhttpLoader {
Err(format!("Failed to load {uri:?}")) Err(format!("Failed to load {uri:?}"))
} }
}; };
let mut cache = cache.lock(); let repaint = {
if let std::collections::hash_map::Entry::Occupied(mut entry) = let mut cache = cache.lock();
cache.entry(uri.clone()) if let std::collections::hash_map::Entry::Occupied(mut entry) =
{ cache.entry(uri.clone())
let entry = entry.get_mut(); {
*entry = Poll::Ready(result); 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(); 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."
);
} }
} }
}); });

View File

@ -95,14 +95,23 @@ impl BytesLoader for FileLoader {
} }
Err(err) => Err(err.to_string()), Err(err) => Err(err.to_string()),
}; };
let mut cache = cache.lock(); let repaint = {
if let std::collections::hash_map::Entry::Occupied(mut entry) = cache.entry(uri.clone()) { let mut cache = cache.lock();
let entry = entry.get_mut(); if let std::collections::hash_map::Entry::Occupied(mut entry) = cache.entry(uri.clone()) {
*entry = Poll::Ready(result); 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(); 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.");
} }
} }
}) })

View File

@ -100,15 +100,28 @@ impl ImageLoader for ImageCrateLoader {
let result = crate::image::load_image_bytes(&bytes) let result = crate::image::load_image_bytes(&bytes)
.map(Arc::new) .map(Arc::new)
.map_err(|err| err.to_string()); .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()) { if let std::collections::hash_map::Entry::Occupied(mut entry) = cache.entry(uri.clone()) {
let entry = entry.get_mut(); let entry = entry.get_mut();
*entry = Poll::Ready(result); *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(); 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.");
} }
} }
}) })

View File

@ -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"))] #[cfg(not(feature = "deadlock_detection"))]
mod mutex_impl { mod mutex_impl {
use super::DEADLOCK_DURATION;
/// Provides interior mutability. /// Provides interior mutability.
/// ///
/// This is a thin wrapper around [`parking_lot::Mutex`], except if /// This is a thin wrapper around [`parking_lot::Mutex`], except if
@ -25,7 +30,7 @@ mod mutex_impl {
pub fn lock(&self) -> MutexGuard<'_, T> { pub fn lock(&self) -> MutexGuard<'_, T> {
if cfg!(debug_assertions) { if cfg!(debug_assertions) {
self.0 self.0
.try_lock_for(std::time::Duration::from_secs(30)) .try_lock_for(DEADLOCK_DURATION)
.expect("Looks like a deadlock!") .expect("Looks like a deadlock!")
} else { } else {
self.0.lock() self.0.lock()
@ -127,6 +132,8 @@ mod mutex_impl {
#[cfg(not(feature = "deadlock_detection"))] #[cfg(not(feature = "deadlock_detection"))]
mod rw_lock_impl { mod rw_lock_impl {
use super::DEADLOCK_DURATION;
/// The lock you get from [`RwLock::read`]. /// The lock you get from [`RwLock::read`].
pub use parking_lot::MappedRwLockReadGuard as RwLockReadGuard; pub use parking_lot::MappedRwLockReadGuard as RwLockReadGuard;
@ -151,12 +158,26 @@ mod rw_lock_impl {
impl<T: ?Sized> RwLock<T> { impl<T: ?Sized> RwLock<T> {
#[inline(always)] #[inline(always)]
pub fn read(&self) -> RwLockReadGuard<'_, T> { 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)] #[inline(always)]
pub fn write(&self) -> RwLockWriteGuard<'_, T> { 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)
} }
} }
} }