From 20ae2b7c72519e17d4c28d9a400a35ed2f39de28 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Fri, 5 Apr 2024 16:14:26 -0700 Subject: [PATCH] map_each deadlock prevention map_each previously was written such that if a chain of mappings fed each other, a deadlock could occur because while the first one was mapped, the second callback gets invoked and tries to update the first value while it's still being held. This refactor switches from std Mutex to parking_lot, allowing me to remove a workaround for needing to run drop callbacks in a separate thread during the drop of a DynamicGuard. In addition to that change, the lower level `map_generational` calls now take a DynamicGuard as their parameter. This allows these functions to drop ownership of the referenced data during the callback. The map_each implementation takes advantage of this by ensuring that the guard is dropped before set is invoked, minimizing potential lock overlaps. With this refactor, some old code of mine with complex validations now works again. --- Cargo.lock | 1 + Cargo.toml | 1 + examples/custom-widgets.rs | 8 +- src/animation.rs | 9 +- src/app.rs | 8 +- src/context.rs | 8 +- src/tick.rs | 8 +- src/tree.rs | 69 ++++++------ src/utils.rs | 15 +-- src/value.rs | 216 +++++++++++++++++++++++++------------ src/widget.rs | 10 +- src/widgets/layers.rs | 6 +- src/widgets/tilemap.rs | 4 +- src/window.rs | 15 ++- 14 files changed, 214 insertions(+), 164 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 030584c..c792a68 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -639,6 +639,7 @@ dependencies = [ "kludgine", "nominals", "palette", + "parking_lot", "plotters", "png", "pollster", diff --git a/Cargo.toml b/Cargo.toml index cb903cb..5bf62c7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -45,6 +45,7 @@ png = "0.17.10" image = { version = "0.25.0", features = ["png"] } plotters = { version = "0.3.5", default-features = false, optional = true } nominals = "0.3.0" +parking_lot = "0.12.1" # [patch.crates-io] diff --git a/examples/custom-widgets.rs b/examples/custom-widgets.rs index 14173ef..0310dfa 100644 --- a/examples/custom-widgets.rs +++ b/examples/custom-widgets.rs @@ -14,11 +14,7 @@ fn main() -> cushy::Result { "Inline Widgets" .and(callback_widget()) .into_rows() - .and( - "impl MakeWidget" - .and(ToggleMakeWidget::default()) - .into_rows(), - ) + .and("impl MakeWidget".and(ToggleMakeWidget).into_rows()) .and("impl Widget".and(impl_widget()).into_rows()) .into_columns() .centered() @@ -59,7 +55,7 @@ fn callback_widget() -> impl MakeWidgetWithTag { /// widget or any of its children aren't focusable, implementing [`MakeWidget`] /// directly will make more sense. #[derive(Default)] -struct ToggleMakeWidget(Toggle); +struct ToggleMakeWidget; impl MakeWidgetWithTag for ToggleMakeWidget { fn make_with_tag(self, id: WidgetTag) -> WidgetInstance { diff --git a/src/animation.rs b/src/animation.rs index 536b92f..a2d6491 100644 --- a/src/animation.rs +++ b/src/animation.rs @@ -43,7 +43,7 @@ use std::cmp::Ordering; use std::fmt::{Debug, Display}; use std::ops::{ControlFlow, Deref, Div, DivAssign, Mul, MulAssign, Sub}; use std::str::FromStr; -use std::sync::{Arc, Condvar, Mutex, MutexGuard, OnceLock}; +use std::sync::{Arc, OnceLock}; use std::thread; use std::time::{Duration, Instant}; @@ -53,10 +53,11 @@ use figures::{Angle, Point, Ranged, Rect, Size, UnscaledUnit, Zero}; use intentional::Cast; use kempt::Set; use kludgine::Color; +use parking_lot::{Condvar, Mutex, MutexGuard}; use crate::animation::easings::Linear; use crate::styles::{Component, RequireInvalidation}; -use crate::utils::{run_in_bg, IgnorePoison}; +use crate::utils::run_in_bg; use crate::value::{Destination, Dynamic, Source}; static ANIMATIONS: Mutex = Mutex::new(Animating::new()); @@ -67,7 +68,7 @@ fn thread_state() -> MutexGuard<'static, Animating> { THREAD.get_or_init(|| { thread::spawn(animation_thread); }); - ANIMATIONS.lock().ignore_poison() + ANIMATIONS.lock() } fn animation_thread() { @@ -75,7 +76,7 @@ fn animation_thread() { loop { if state.running.is_empty() { state.last_updated = None; - state = NEW_ANIMATIONS.wait(state).ignore_poison(); + NEW_ANIMATIONS.wait(&mut state); } else { let start = Instant::now(); let last_tick = state.last_updated.unwrap_or(start); diff --git a/src/app.rs b/src/app.rs index c383d0f..8807e2f 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1,10 +1,10 @@ -use std::sync::{Arc, Mutex, MutexGuard}; +use std::sync::Arc; use arboard::Clipboard; use kludgine::app::{AppEvent, AsApplication}; +use parking_lot::{Mutex, MutexGuard}; use crate::fonts::FontCollection; -use crate::utils::IgnorePoison; use crate::window::sealed::WindowCommand; use crate::window::WindowHandle; @@ -64,9 +64,7 @@ impl Cushy { /// initialized when the window opened. #[must_use] pub fn clipboard_guard(&self) -> Option> { - self.clipboard - .as_ref() - .map(|mutex| mutex.lock().ignore_poison()) + self.clipboard.as_ref().map(|mutex| mutex.lock()) } /// Returns the font collection that will be loaded in all Cushy windows. diff --git a/src/context.rs b/src/context.rs index 6af6dbe..c3d58e9 100644 --- a/src/context.rs +++ b/src/context.rs @@ -1414,11 +1414,11 @@ impl Trackable for T where T: sealed::Trackable {} pub(crate) mod sealed { use std::sync::atomic::{AtomicBool, Ordering}; - use std::sync::{Arc, Mutex, MutexGuard}; + use std::sync::Arc; use kempt::Set; + use parking_lot::{Mutex, MutexGuard}; - use crate::utils::IgnorePoison; use crate::widget::WidgetId; use crate::window::WindowHandle; @@ -1445,12 +1445,12 @@ pub(crate) mod sealed { } pub fn invalidate(&self, widget: WidgetId) -> bool { - let mut invalidated = self.invalidated.lock().ignore_poison(); + let mut invalidated = self.invalidated.lock(); invalidated.insert(widget) } pub fn invalidations(&self) -> MutexGuard<'_, Set> { - self.invalidated.lock().ignore_poison() + self.invalidated.lock() } } diff --git a/src/tick.rs b/src/tick.rs index 6120618..a884f56 100644 --- a/src/tick.rs +++ b/src/tick.rs @@ -1,5 +1,5 @@ use std::sync::atomic::{AtomicUsize, Ordering}; -use std::sync::{Arc, Condvar, Mutex, MutexGuard}; +use std::sync::Arc; use std::time::{Duration, Instant}; use ahash::AHashSet; @@ -8,9 +8,9 @@ use figures::Point; use intentional::Assert; use kludgine::app::winit::event::{ElementState, MouseButton}; use kludgine::app::winit::keyboard::Key; +use parking_lot::{Condvar, Mutex, MutexGuard}; use crate::context::WidgetContext; -use crate::utils::IgnorePoison; use crate::value::{Destination, Dynamic}; use crate::widget::{EventHandling, HANDLED, IGNORED}; use crate::window::KeyEvent; @@ -170,7 +170,7 @@ struct TickData { impl TickData { fn state(&self) -> MutexGuard<'_, TickState> { - self.state.lock().ignore_poison() + self.state.lock() } } @@ -218,7 +218,7 @@ where while state.keep_running { let current_frame = data.rendered_frame.load(Ordering::Acquire); if state.frame == current_frame { - state = data.sync.wait(state).ignore_poison(); + data.sync.wait(&mut state); } else { break; } diff --git a/src/tree.rs b/src/tree.rs index 5f4aa78..9de8bb0 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -1,13 +1,13 @@ use std::mem; -use std::sync::{Arc, Mutex, Weak}; +use std::sync::{Arc, Weak}; use ahash::AHashMap; use alot::{LotId, Lots}; use figures::units::{Px, UPx}; use figures::{Point, Rect, Size}; +use parking_lot::Mutex; use crate::styles::{Styles, ThemePair, VisualOrder}; -use crate::utils::IgnorePoison; use crate::value::Value; use crate::widget::{MountedWidget, WidgetId, WidgetInstance}; use crate::window::{ThemeMode, WindowHandle}; @@ -24,7 +24,7 @@ impl Tree { widget: WidgetInstance, parent: Option<&MountedWidget>, ) -> MountedWidget { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); let id = widget.id(); let (effective_styles, parent_id) = if let Some(parent) = parent { ( @@ -72,7 +72,7 @@ impl Tree { parent: &MountedWidget, children_to_unmount: &mut Vec, ) { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); data.remove_child(child.node_id, parent.node_id, children_to_unmount); if child.widget.is_default() { @@ -84,7 +84,7 @@ impl Tree { } pub(crate) fn set_layout(&self, widget: LotId, rect: Rect) { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); let node = &mut data.nodes[widget]; node.layout = Some(rect); @@ -104,12 +104,12 @@ impl Tree { } pub(crate) fn layout(&self, widget: LotId) -> Option> { - let data = self.data.lock().ignore_poison(); + let data = self.data.lock(); data.nodes.get(widget).and_then(|widget| widget.layout) } pub(crate) fn new_frame(&self, invalidations: impl IntoIterator) { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); data.render_info.clear(); for id in invalidations { @@ -122,7 +122,7 @@ impl Tree { } pub(crate) fn note_widget_rendered(&self, widget: LotId) { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); let Some(layout) = data.nodes.get(widget).and_then(|node| node.layout) else { return; }; @@ -134,7 +134,7 @@ impl Tree { parent: LotId, constraints: Size, ) -> Option> { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); let node = &mut data.nodes[parent]; if let Some(cached_layout) = &node.last_layout_query { @@ -161,7 +161,7 @@ impl Tree { constraints: Size, size: Size, ) { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); data.nodes[id].last_layout_query = Some(CachedLayoutQuery { constraints, size }); } @@ -170,7 +170,7 @@ impl Tree { parent: LotId, order: VisualOrder, ) -> Vec { - let data = self.data.lock().ignore_poison(); + let data = self.data.lock(); let node = &data.nodes[parent]; let mut unordered = node.children.clone(); let mut ordered = Vec::::with_capacity(unordered.len()); @@ -230,12 +230,12 @@ impl Tree { } pub(crate) fn effective_styles(&self, id: LotId) -> Styles { - let data = self.data.lock().ignore_poison(); + let data = self.data.lock(); data.nodes[id].effective_styles.clone() } pub(crate) fn hover(&self, new_hover: Option<&MountedWidget>) -> HoverResults { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); let hovered = new_hover .map(|new_hover| data.widget_hierarchy(new_hover.node_id, self)) .unwrap_or_default(); @@ -260,12 +260,12 @@ impl Tree { } pub fn focus(&self, new_focus: Option) -> Result, ()> { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); data.update_tracked_widget(new_focus, self, |data| &mut data.focus) } pub fn previous_focus(&self, focus: WidgetId) -> Option { - let data = self.data.lock().ignore_poison(); + let data = self.data.lock(); let previous = *data.previous_focuses.get(&focus)?; data.widget_from_id(previous, self) } @@ -274,24 +274,24 @@ impl Tree { &self, new_active: Option<&MountedWidget>, ) -> Result, ()> { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); data.update_tracked_widget(new_active.map(MountedWidget::id), self, |data| { &mut data.active }) } pub fn widget(&self, id: WidgetId) -> Option { - let data = self.data.lock().ignore_poison(); + let data = self.data.lock(); data.widget_from_id(id, self) } pub(crate) fn widget_from_node(&self, id: LotId) -> Option { - let data = self.data.lock().ignore_poison(); + let data = self.data.lock(); data.widget_from_node(id, self) } pub(crate) fn is_enabled(&self, mut id: LotId, context: &WindowHandle) -> bool { - let data = self.data.lock().ignore_poison(); + let data = self.data.lock(); loop { let Some(node) = data.nodes.get(id) else { return false; @@ -310,23 +310,23 @@ impl Tree { } pub(crate) fn active_widget(&self) -> Option { - self.data.lock().ignore_poison().active + self.data.lock().active } pub(crate) fn hovered_widget(&self) -> Option { - self.data.lock().ignore_poison().hover + self.data.lock().hover } pub(crate) fn default_widget(&self) -> Option { - self.data.lock().ignore_poison().defaults.last().copied() + self.data.lock().defaults.last().copied() } pub(crate) fn escape_widget(&self) -> Option { - self.data.lock().ignore_poison().escapes.last().copied() + self.data.lock().escapes.last().copied() } pub(crate) fn is_hovered(&self, id: LotId) -> bool { - let data = self.data.lock().ignore_poison(); + let data = self.data.lock(); let mut search = data.hover; while let Some(hovered) = search { if hovered == id { @@ -339,21 +339,21 @@ impl Tree { } pub(crate) fn focused_widget(&self) -> Option { - self.data.lock().ignore_poison().focus + self.data.lock().focus } pub(crate) fn widgets_under_point(&self, point: Point) -> Vec { - let data = self.data.lock().ignore_poison(); + let data = self.data.lock(); data.render_info.widgets_under_point(point, &data, self) } pub(crate) fn parent(&self, id: LotId) -> Option { - let data = self.data.lock().ignore_poison(); + let data = self.data.lock(); data.nodes.get(id).expect("missing widget").parent } pub(crate) fn is_child(&self, mut id: LotId, possible_parent: &WidgetInstance) -> bool { - let data = self.data.lock().ignore_poison(); + let data = self.data.lock(); while let Some(node) = data.nodes.get(id) { if &node.widget == possible_parent { return true; @@ -371,17 +371,17 @@ impl Tree { } pub(crate) fn attach_styles(&self, id: LotId, styles: Value) { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); data.attach_styles(id, styles); } pub(crate) fn attach_theme(&self, id: LotId, theme: Value) { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); data.nodes.get_mut(id).expect("missing widget").theme = Some(theme); } pub(crate) fn attach_theme_mode(&self, id: LotId, theme: Value) { - let mut data = self.data.lock().ignore_poison(); + let mut data = self.data.lock(); data.nodes.get_mut(id).expect("missing widget").theme_mode = Some(theme); } @@ -389,7 +389,7 @@ impl Tree { &self, id: LotId, ) -> (Styles, Option>, Option>) { - let data = self.data.lock().ignore_poison(); + let data = self.data.lock(); let node = data.nodes.get(id).expect("missing widget"); ( node.effective_styles.clone(), @@ -399,10 +399,7 @@ impl Tree { } pub fn invalidate(&self, id: LotId, include_hierarchy: bool) { - self.data - .lock() - .ignore_poison() - .invalidate(id, include_hierarchy); + self.data.lock().invalidate(id, include_hierarchy); } } diff --git a/src/utils.rs b/src/utils.rs index ad34001..b244c0c 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,6 +1,6 @@ use std::ops::Deref; use std::sync::mpsc::{self, SyncSender}; -use std::sync::{OnceLock, PoisonError}; +use std::sync::OnceLock; use intentional::Assert; use kludgine::app::winit::event::Modifiers; @@ -225,19 +225,6 @@ impl Deref for Lazy { } } -pub trait IgnorePoison { - type Unwrapped; - fn ignore_poison(self) -> Self::Unwrapped; -} - -impl IgnorePoison for Result> { - type Unwrapped = T; - - fn ignore_poison(self) -> Self::Unwrapped { - self.map_or_else(PoisonError::into_inner, |g| g) - } -} - pub trait BgFunction: FnOnce() + Send + 'static {} pub fn run_in_bg(f: F) diff --git a/src/value.rs b/src/value.rs index 3ab8f47..4d2a714 100644 --- a/src/value.rs +++ b/src/value.rs @@ -7,7 +7,7 @@ use std::future::Future; use std::hash::{BuildHasher, Hash}; use std::ops::{Add, AddAssign, Deref, DerefMut, Not}; use std::str::FromStr; -use std::sync::{Arc, Condvar, Mutex, MutexGuard, TryLockError, Weak}; +use std::sync::{Arc, Weak}; use std::task::{Poll, Waker}; use std::thread::{self, ThreadId}; use std::time::{Duration, Instant}; @@ -16,10 +16,11 @@ use ahash::AHashSet; use alot::{LotId, Lots}; use intentional::Assert; use kempt::{Map, Sort}; +use parking_lot::{Condvar, Mutex, MutexGuard}; use crate::animation::{AnimationHandle, DynamicTransition, IntoAnimate, LinearInterpolate, Spawn}; use crate::context::{self, Trackable, WidgetContext}; -use crate::utils::{run_in_bg, IgnorePoison, WithClone}; +use crate::utils::WithClone; use crate::widget::{ MakeWidget, MakeWidgetWithTag, OnceCallback, WidgetId, WidgetInstance, WidgetList, }; @@ -32,7 +33,7 @@ pub trait Source { /// [`Generation`]. fn try_map_generational( &self, - map: impl FnOnce(&GenerationalValue) -> R, + map: impl FnOnce(DynamicGuard<'_, T, true>) -> R, ) -> Result; /// Maps the contents with read-only access, providing access to the value's @@ -42,7 +43,7 @@ pub trait Source { /// /// This function panics if this value is already locked by the current /// thread. - fn map_generational(&self, map: impl FnOnce(&GenerationalValue) -> R) -> R { + fn map_generational(&self, map: impl FnOnce(DynamicGuard<'_, T, true>) -> R) -> R { self.try_map_generational(map).expect("deadlocked") } @@ -54,7 +55,7 @@ pub trait Source { /// thread. #[must_use] fn generation(&self) -> Generation { - self.map_generational(GenerationalValue::generation) + self.map_generational(|g| g.generation()) } /// Maps the contents with read-only access. @@ -64,7 +65,7 @@ pub trait Source { /// This function panics if this value is already locked by the current /// thread. fn map_ref(&self, map: impl FnOnce(&T) -> R) -> R { - self.map_generational(|gen| map(&gen.value)) + self.map_generational(|gen| map(&*gen)) } /// Returns a clone of the currently contained value. @@ -83,7 +84,7 @@ pub trait Source { /// Maps the contents with read-only access. fn try_map_ref(&self, map: impl FnOnce(&T) -> R) -> Result { - self.try_map_generational(|gen| map(&gen.value)) + self.try_map_generational(|gen| map(&*gen)) } /// Returns a clone of the currently contained value. @@ -91,7 +92,7 @@ pub trait Source { where T: Clone, { - self.try_map_generational(|gen| gen.value.clone()) + self.try_map_generational(|gen| gen.clone()) } /// Returns a clone of the currently contained value. @@ -138,7 +139,7 @@ pub trait Source { fn for_each_generational_try(&self, for_each: F) -> CallbackHandle where T: Send + 'static, - F: for<'a> FnMut(&'a GenerationalValue) -> Result<(), CallbackDisconnected> + F: for<'a> FnMut(DynamicGuard<'_, T, true>) -> Result<(), CallbackDisconnected> + Send + 'static; @@ -147,7 +148,7 @@ pub trait Source { fn for_each_generational(&self, mut for_each: F) -> CallbackHandle where T: Send + 'static, - F: for<'a> FnMut(&'a GenerationalValue) + Send + 'static, + F: for<'a> FnMut(DynamicGuard<'_, T, true>) + Send + 'static, { self.for_each_generational_try(move |value| { for_each(value); @@ -165,7 +166,7 @@ pub trait Source { T: Send + 'static, F: for<'a> FnMut(&'a T) -> Result<(), CallbackDisconnected> + Send + 'static, { - self.for_each_generational_try(move |gen| for_each(&gen.value)) + self.for_each_generational_try(move |gen| for_each(&*gen)) } /// Attaches `for_each` to this value so that it is invoked each time the @@ -248,7 +249,7 @@ pub trait Source { fn map_each_generational(&self, mut map: F) -> Dynamic where T: Send + 'static, - F: for<'a> FnMut(&'a GenerationalValue) -> R + Send + 'static, + F: for<'a> FnMut(DynamicGuard<'a, T, true>) -> R + Send + 'static, R: PartialEq + Send + 'static, { let mapped = Dynamic::new(self.map_generational(&mut map)); @@ -269,7 +270,7 @@ pub trait Source { F: for<'a> FnMut(&'a T) -> R + Send + 'static, R: PartialEq + Send + 'static, { - self.map_each_generational(move |gen| map(&gen.value)) + self.map_each_generational(move |gen| map(&*gen)) } /// Creates a new dynamic value that contains the result of invoking `map` @@ -520,16 +521,20 @@ pub trait Destination { impl Source for Arc> { fn try_map_generational( &self, - map: impl FnOnce(&GenerationalValue) -> R, + map: impl FnOnce(DynamicGuard<'_, T, true>) -> R, ) -> Result { let state = self.state()?; - Ok(map(&state.wrapped)) + Ok(map(DynamicGuard { + guard: DynamicOrOwnedGuard::Dynamic(state), + accessed_mut: false, + prevent_notifications: false, + })) } fn for_each_generational_try(&self, mut for_each: F) -> CallbackHandle where T: Send + 'static, - F: for<'a> FnMut(&'a GenerationalValue) -> Result<(), CallbackDisconnected> + F: for<'a> FnMut(DynamicGuard<'a, T, true>) -> Result<(), CallbackDisconnected> + Send + 'static, { @@ -550,7 +555,7 @@ impl Source for Arc> { dynamic_for_each(self, move || { let this = this.upgrade().ok_or(CallbackDisconnected)?; - if let Ok(value) = this.try_map_generational(GenerationalValue::clone) { + if let Ok(value) = this.try_map_generational(|g| g.guard.clone()) { for_each(value)?; } @@ -562,7 +567,7 @@ impl Source for Arc> { impl Source for Dynamic { fn try_map_generational( &self, - map: impl FnOnce(&GenerationalValue) -> R, + map: impl FnOnce(DynamicGuard<'_, T, true>) -> R, ) -> Result { self.0.try_map_generational(map) } @@ -570,7 +575,7 @@ impl Source for Dynamic { fn for_each_generational_try(&self, for_each: F) -> CallbackHandle where T: Send + 'static, - F: for<'a> FnMut(&'a GenerationalValue) -> Result<(), CallbackDisconnected> + F: for<'a> FnMut(DynamicGuard<'_, T, true>) -> Result<(), CallbackDisconnected> + Send + 'static, { @@ -589,10 +594,10 @@ impl Source for Dynamic { impl Source for DynamicReader { fn try_map_generational( &self, - map: impl FnOnce(&GenerationalValue) -> R, + map: impl FnOnce(DynamicGuard<'_, T, true>) -> R, ) -> Result { self.source.try_map_generational(|generational| { - *self.read_generation.lock().ignore_poison() = generational.generation; + *self.read_generation.lock() = generational.generation(); map(generational) }) } @@ -600,7 +605,7 @@ impl Source for DynamicReader { fn for_each_generational_try(&self, for_each: F) -> CallbackHandle where T: Send + 'static, - F: for<'a> FnMut(&'a GenerationalValue) -> Result<(), CallbackDisconnected> + F: for<'a> FnMut(DynamicGuard<'_, T, true>) -> Result<(), CallbackDisconnected> + Send + 'static, { @@ -735,19 +740,23 @@ impl Owned { impl Source for Owned { fn try_map_generational( &self, - map: impl FnOnce(&GenerationalValue) -> R, + map: impl FnOnce(DynamicGuard<'_, T, true>) -> R, ) -> Result { - Ok(map(&self.wrapped.borrow())) + Ok(map(DynamicGuard { + guard: DynamicOrOwnedGuard::Owned(self.wrapped.borrow_mut()), + accessed_mut: false, + prevent_notifications: false, + })) } fn for_each_generational_try(&self, for_each: F) -> CallbackHandle where T: Send + 'static, - F: for<'a> FnMut(&'a GenerationalValue) -> Result<(), CallbackDisconnected> + F: for<'a> FnMut(DynamicGuard<'a, T, true>) -> Result<(), CallbackDisconnected> + Send + 'static, { - let mut callbacks = self.callbacks.active.lock().ignore_poison(); + let mut callbacks = self.callbacks.active.lock(); CallbackHandle(CallbackHandleInner::Single(CallbackHandleData { id: Some(callbacks.push(Box::new(for_each))), owner: None, @@ -760,7 +769,7 @@ impl Source for Owned { T: Clone + Send + 'static, F: FnMut(GenerationalValue) -> Result<(), CallbackDisconnected> + Send + 'static, { - self.for_each_generational_try(move |value| for_each(value.clone())) + self.for_each_generational_try(move |gen| for_each(gen.guard.clone())) } } @@ -775,7 +784,9 @@ where &mut updated, )); if updated { - self.callbacks.invoke(&self.wrapped.borrow()); + self.callbacks.invoke(&mut &self.wrapped, |wrapped| { + DynamicOrOwnedGuard::Owned(wrapped.borrow_mut()) + }); } Ok(result) } @@ -829,7 +840,9 @@ where { fn drop(&mut self) { if self.accessed_mut { - self.owned.callbacks.invoke(&self.borrowed); + self.owned.callbacks.invoke(&mut self.borrowed, |borrowed| { + DynamicOrOwnedGuard::OwnedRef(&mut *borrowed) + }); } } } @@ -850,9 +863,21 @@ impl OwnedCallbacks where T: 'static, { - pub fn invoke(&self, value: &GenerationalValue) { - let mut callbacks = self.active.lock().ignore_poison(); - callbacks.drain_filter(|callback| callback.updated(value).is_err()); + pub fn invoke<'a, U>( + &self, + user: &'a mut U, + value: impl for<'b> Fn(&'b mut U) -> DynamicOrOwnedGuard<'b, T>, + ) { + let mut callbacks = self.active.lock(); + callbacks.drain_filter(|callback| { + callback + .updated(DynamicGuard { + guard: value(user), + accessed_mut: false, + prevent_notifications: false, + }) + .is_err() + }); } } @@ -861,19 +886,21 @@ where T: 'static, { fn remove(&self, id: LotId) { - self.active.lock().ignore_poison().remove(id); + self.active.lock().remove(id); } } trait OwnedCallbackFn: Send + 'static { - fn updated(&mut self, value: &GenerationalValue) -> Result<(), CallbackDisconnected>; + fn updated(&mut self, value: DynamicGuard<'_, T, true>) -> Result<(), CallbackDisconnected>; } impl OwnedCallbackFn for F where - F: for<'a> FnMut(&'a GenerationalValue) -> Result<(), CallbackDisconnected> + Send + 'static, + F: for<'a> FnMut(DynamicGuard<'a, T, true>) -> Result<(), CallbackDisconnected> + + Send + + 'static, { - fn updated(&mut self, value: &GenerationalValue) -> Result<(), CallbackDisconnected> { + fn updated(&mut self, value: DynamicGuard<'_, T, true>) -> Result<(), CallbackDisconnected> { self(value) } } @@ -1104,7 +1131,7 @@ impl Dynamic { /// dynamic. pub fn try_lock(&self) -> Result, DeadlockError> { Ok(DynamicGuard { - guard: self.0.state()?, + guard: DynamicOrOwnedGuard::Dynamic(self.0.state()?), accessed_mut: false, prevent_notifications: false, }) @@ -1112,7 +1139,7 @@ impl Dynamic { fn lock_inner(&self) -> DynamicGuard<'_, T, READONLY> { DynamicGuard { - guard: self.0.state().expect("deadlocked"), + guard: DynamicOrOwnedGuard::Dynamic(self.0.state().expect("deadlocked")), accessed_mut: false, prevent_notifications: false, } @@ -1344,9 +1371,19 @@ where } } +impl<'a, T> DynamicMutexGuard<'a, T> { + fn unlocked(&mut self, while_unlocked: impl FnOnce()) { + MutexGuard::unlocked(&mut self.guard, || { + let current_state = self.dynamic.during_callback_state.lock().take(); + while_unlocked(); + *self.dynamic.during_callback_state.lock() = current_state; + }); + } +} + impl<'a, T> Drop for DynamicMutexGuard<'a, T> { fn drop(&mut self) { - let mut during_state = self.dynamic.during_callback_state.lock().ignore_poison(); + let mut during_state = self.dynamic.during_callback_state.lock(); *during_state = None; drop(during_state); self.dynamic.sync.notify_all(); @@ -1379,20 +1416,19 @@ struct DynamicData { impl DynamicData { fn state(&self) -> Result, DeadlockError> { - let mut during_sync = self.during_callback_state.lock().ignore_poison(); + let mut during_sync = self.during_callback_state.lock(); let current_thread_id = std::thread::current().id(); let guard = loop { match self.state.try_lock() { - Ok(g) => break g, - Err(TryLockError::Poisoned(poision)) => break poision.into_inner(), - Err(TryLockError::WouldBlock) => loop { + Some(g) => break g, + None => loop { match &*during_sync { Some(state) if state.locked_thread == current_thread_id => { return Err(DeadlockError) } Some(_) => { - during_sync = self.sync.wait(during_sync).ignore_poison(); + self.sync.wait(&mut during_sync); } None => break, } @@ -1443,7 +1479,7 @@ where T: Send + 'static, { let state = this.state().expect("deadlocked"); - let mut data = state.callbacks.callbacks.lock().ignore_poison(); + let mut data = state.callbacks.callbacks.lock(); CallbackHandle(CallbackHandleInner::Single(CallbackHandleData { id: Some(data.callbacks.push(Box::new(map))), owner: Some(this.clone()), @@ -1816,7 +1852,7 @@ struct ChangeCallbacksData { impl CallbackCollection for ChangeCallbacksData { fn remove(&self, id: LotId) { - let mut data = self.callbacks.lock().ignore_poison(); + let mut data = self.callbacks.lock(); data.callbacks.remove(id); } } @@ -1842,7 +1878,7 @@ struct ChangeCallbacks { impl Drop for ChangeCallbacks { fn drop(&mut self) { - let mut currently_executing = self.data.currently_executing.lock().expect("lock poisoned"); + let mut currently_executing = self.data.currently_executing.lock(); let current_thread = thread::current().id(); loop { match &*currently_executing { @@ -1854,7 +1890,7 @@ impl Drop for ChangeCallbacks { drop(currently_executing); // Invoke the callbacks - let mut state = self.data.callbacks.lock().ignore_poison(); + let mut state = self.data.callbacks.lock(); // If the callbacks have already been invoked by another // thread such that the callbacks observed the value our // thread wrote, we can skip the callbacks. @@ -1870,8 +1906,7 @@ impl Drop for ChangeCallbacks { // Remove ourselves as the current executor, notifying any // other threads that are waiting. - currently_executing = - self.data.currently_executing.lock().expect("lock poisoned"); + currently_executing = self.data.currently_executing.lock(); *currently_executing = None; drop(currently_executing); self.data.sync.notify_all(); @@ -1886,11 +1921,7 @@ impl Drop for ChangeCallbacks { return; } Some(_) => { - currently_executing = self - .data - .sync - .wait(currently_executing) - .expect("lock poisoned"); + self.data.sync.wait(&mut currently_executing); } } } @@ -1964,13 +1995,57 @@ impl DerefMut for GenerationalValue { } } +#[derive(Debug)] +enum DynamicOrOwnedGuard<'a, T> { + Dynamic(DynamicMutexGuard<'a, T>), + Owned(RefMut<'a, GenerationalValue>), + OwnedRef(&'a mut GenerationalValue), +} +impl<'a, T> DynamicOrOwnedGuard<'a, T> { + fn note_changed(&mut self) -> Option { + match self { + Self::Dynamic(guard) => Some(guard.note_changed()), + Self::Owned(_) | Self::OwnedRef(_) => None, + } + } + + fn unlocked(&mut self, while_unlocked: impl FnOnce()) { + match self { + Self::Dynamic(guard) => guard.unlocked(while_unlocked), + Self::Owned(_) | Self::OwnedRef(_) => while_unlocked(), + } + } +} + +impl<'a, T> Deref for DynamicOrOwnedGuard<'a, T> { + type Target = GenerationalValue; + + fn deref(&self) -> &Self::Target { + match self { + Self::Dynamic(guard) => &guard.wrapped, + Self::Owned(r) => r, + Self::OwnedRef(r) => r, + } + } +} + +impl<'a, T> DerefMut for DynamicOrOwnedGuard<'a, T> { + fn deref_mut(&mut self) -> &mut Self::Target { + match self { + Self::Dynamic(guard) => &mut guard.wrapped, + Self::Owned(r) => r, + Self::OwnedRef(r) => r, + } + } +} + /// An exclusive reference to the contents of a [`Dynamic`]. /// /// If the contents are accessed through [`DerefMut`], all obververs will be /// notified of a change when this guard is dropped. #[derive(Debug)] pub struct DynamicGuard<'a, T, const READONLY: bool = false> { - guard: DynamicMutexGuard<'a, T>, + guard: DynamicOrOwnedGuard<'a, T>, accessed_mut: bool, prevent_notifications: bool, } @@ -1982,7 +2057,7 @@ impl DynamicGuard<'_, T, READONLY> { /// will remain unchanged while the guard is held. #[must_use] pub fn generation(&self) -> Generation { - self.guard.wrapped.generation + self.guard.generation } /// Prevent any access through [`DerefMut`] from triggering change @@ -1996,22 +2071,22 @@ impl<'a, T, const READONLY: bool> Deref for DynamicGuard<'a, T, READONLY> { type Target = T; fn deref(&self) -> &Self::Target { - &self.guard.wrapped.value + &self.guard.value } } impl<'a, T> DerefMut for DynamicGuard<'a, T, false> { fn deref_mut(&mut self) -> &mut Self::Target { self.accessed_mut = true; - &mut self.guard.wrapped.value + &mut self.guard.value } } impl Drop for DynamicGuard<'_, T, READONLY> { fn drop(&mut self) { if self.accessed_mut && !self.prevent_notifications { - let mut callbacks = Some(self.guard.note_changed()); - run_in_bg(move || drop(callbacks.take())); + let callbacks = self.guard.note_changed(); + self.guard.unlocked(|| drop(callbacks)); } } } @@ -2103,7 +2178,7 @@ impl DynamicReader { #[must_use] pub fn lock(&self) -> DynamicGuard<'_, T, true> { DynamicGuard { - guard: self.source.state().expect("deadlocked"), + guard: DynamicOrOwnedGuard::Dynamic(self.source.state().expect("deadlocked")), accessed_mut: false, prevent_notifications: false, } @@ -2113,7 +2188,7 @@ impl DynamicReader { /// reader. #[must_use] pub fn read_generation(&self) -> Generation { - *self.read_generation.lock().ignore_poison() + *self.read_generation.lock() } /// Returns true if the dynamic has been modified since the last time the @@ -2142,13 +2217,12 @@ impl DynamicReader { self.source .during_callback_state .lock() - .ignore_poison() .as_ref() .map_or(true, |state| state.locked_thread != std::thread::current().id()), "deadlocked" ); - let mut state = self.source.state.lock().ignore_poison(); + let mut state = self.source.state.lock(); loop { if state.wrapped.generation != self.read_generation() { return true; @@ -2159,14 +2233,14 @@ impl DynamicReader { } // Wait for a notification of a change, which is synch - state = self.source.sync.wait(state).ignore_poison(); + self.source.sync.wait(&mut state); } } /// Returns true if this reader still has any writers connected to it. #[must_use] pub fn connected(&self) -> bool { - let state = self.source.state.lock().ignore_poison(); + let state = self.source.state.lock(); state.readers < Arc::strong_count(&self.source) && state.on_disconnect.is_some() } @@ -2936,7 +3010,7 @@ macro_rules! impl_tuple_for_each { move |$var: &$type| { $(let $rvar = $rvar.read();)+ let mut for_each = - for_each.lock().ignore_poison(); + for_each.lock(); (for_each)(($(&$avar,)+)); } })); @@ -3108,7 +3182,7 @@ macro_rules! impl_tuple_for_each_cloned { $handles += $var.for_each_cloned((&$for_each, $(&$rvar,)+).with_clone(|(for_each, $($rvar,)+)| { move |$var: $type| { $(let $rvar = $rvar.get();)+ - if let Ok(mut for_each) = + if let Some(mut for_each) = for_each.try_lock() { (for_each)(($($avar,)+)); } @@ -3259,7 +3333,7 @@ impl Validations { { let validation = Dynamic::new(Validation::None); let mut message_mapping = Self::map_to_message(move |value| check(value)); - let error_message = dynamic.map_each_generational(move |value| message_mapping(value)); + let error_message = dynamic.map_each_generational(move |gen| message_mapping(&gen.guard)); validation.set_source((&self.state, &error_message).for_each_cloned({ let mut f = self.generate_validation(dynamic); @@ -3453,7 +3527,7 @@ impl WhenValidation<'_> { let validation = Dynamic::new(Validation::None); let mut map_to_message = Validations::map_to_message(move |value| check(value)); let error_message = - dynamic.map_each_generational(move |generational| map_to_message(generational)); + dynamic.map_each_generational(move |generational| map_to_message(&generational.guard)); let mut f = self.validations.generate_validation(dynamic); let not = self.not; diff --git a/src/widget.rs b/src/widget.rs index 3b9f362..49a5a6d 100644 --- a/src/widget.rs +++ b/src/widget.rs @@ -5,7 +5,7 @@ use std::clone::Clone; use std::fmt::{self, Debug}; use std::ops::{ControlFlow, Deref, DerefMut}; use std::sync::atomic::{self, AtomicU64}; -use std::sync::{Arc, Mutex, MutexGuard}; +use std::sync::Arc; use std::{slice, vec}; use alot::LotId; @@ -15,6 +15,7 @@ use intentional::Assert; use kludgine::app::winit::event::{Ime, MouseButton, MouseScrollDelta, TouchPhase}; use kludgine::app::winit::window::CursorIcon; use kludgine::Color; +use parking_lot::{Mutex, MutexGuard}; use crate::app::{Application, Open, PendingApp, Run}; use crate::context::sealed::Trackable as _; @@ -35,7 +36,6 @@ use crate::styles::{ IntoDynamicComponentValue, Styles, ThemePair, VisualOrder, }; use crate::tree::{Tree, WeakTree}; -use crate::utils::IgnorePoison; use crate::value::{Dynamic, Generation, IntoDynamic, IntoValue, Validation, Value}; use crate::widgets::checkbox::{Checkable, CheckboxState}; use crate::widgets::layers::{OverlayLayer, Tooltipped}; @@ -1404,8 +1404,8 @@ pub struct WidgetInstance { impl Debug for WidgetInstance { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self.data.widget.try_lock() { - Ok(widget) => widget.summarize(f), - Err(_) => f.debug_struct("WidgetInstance").finish_non_exhaustive(), + Some(widget) => widget.summarize(f), + None => f.debug_struct("WidgetInstance").finish_non_exhaustive(), } } } @@ -1539,7 +1539,7 @@ impl WidgetInstance { /// occur due to other widget locks being held. #[must_use] pub fn lock(&self) -> WidgetGuard<'_> { - WidgetGuard(self.data.widget.lock().ignore_poison()) + WidgetGuard(self.data.widget.lock()) } /// Returns the id of the widget that should receive focus after this diff --git a/src/widgets/layers.rs b/src/widgets/layers.rs index 684b937..97b9dd9 100644 --- a/src/widgets/layers.rs +++ b/src/widgets/layers.rs @@ -1,7 +1,7 @@ //! Widgets that stack in the Z-direction. use std::fmt::{self, Debug}; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use std::time::Duration; use alot::{LotId, OrderedLots}; @@ -9,11 +9,11 @@ use cushy::widget::{RootBehavior, WidgetInstance}; use figures::units::{Lp, Px, UPx}; use figures::{IntoSigned, IntoUnsigned, Point, Rect, Size, Zero}; use intentional::Assert; +use parking_lot::Mutex; use crate::animation::easings::EaseOutQuadradic; use crate::animation::{AnimationHandle, AnimationTarget, IntoAnimate, Spawn, ZeroToOne}; use crate::context::{AsEventContext, EventContext, GraphicsContext, LayoutContext, Trackable}; -use crate::utils::IgnorePoison; use crate::value::{Destination, Dynamic, DynamicGuard, IntoValue, Source, Value}; use crate::widget::{ Callback, MakeWidget, MountedChildren, MountedWidget, Widget, WidgetId, WidgetList, WidgetRef, @@ -641,7 +641,7 @@ struct OverlayLayout { impl Drop for OverlayLayout { fn drop(&mut self) { if let Some(on_dismiss) = &self.on_dismiss { - let mut on_dismiss = on_dismiss.lock().ignore_poison(); + let mut on_dismiss = on_dismiss.lock(); on_dismiss.invoke(()); } } diff --git a/src/widgets/tilemap.rs b/src/widgets/tilemap.rs index 80430e5..69205a4 100644 --- a/src/widgets/tilemap.rs +++ b/src/widgets/tilemap.rs @@ -144,9 +144,7 @@ where fn hover(&mut self, local: Point, context: &mut EventContext<'_>) -> Option { if let Some(tick) = &self.tick { - let Some(size) = context.last_layout().map(|rect| rect.size) else { - return None; - }; + let size = context.last_layout().map(|rect| rect.size)?; let world = tilemap::translate_coordinates(local, context.kludgine.scale(), self.zoom, size); diff --git a/src/window.rs b/src/window.rs index 02c04f5..51e8fc6 100644 --- a/src/window.rs +++ b/src/window.rs @@ -11,7 +11,7 @@ use std::num::{NonZeroU32, TryFromIntError}; use std::ops::{Deref, DerefMut, Not}; use std::path::Path; use std::string::ToString; -use std::sync::{mpsc, Arc, Mutex, MutexGuard, OnceLock}; +use std::sync::{mpsc, Arc, OnceLock}; use std::time::{Duration, Instant}; use ahash::AHashMap; @@ -37,6 +37,7 @@ use kludgine::drawing::Drawing; use kludgine::shapes::Shape; use kludgine::wgpu::{self, CompositeAlphaMode, COPY_BYTES_PER_ROW_ALIGNMENT}; use kludgine::{Color, DrawableExt, Kludgine, KludgineId, Origin, Texture}; +use parking_lot::{Mutex, MutexGuard}; use tracing::Level; use unicode_segmentation::UnicodeSegmentation; @@ -2216,11 +2217,7 @@ impl InnerWindowHandle { if let Some(handle) = pending.handle.get() { let _result = handle.send(message); } else { - pending - .commands - .lock() - .expect("lock poisoned") - .push(message); + pending.commands.lock().push(message); } } InnerWindowHandle::Known(handle) => { @@ -2287,7 +2284,7 @@ impl PendingWindow { let initialized = pending.handle.set(handle.clone()); assert!(initialized.is_ok()); - for command in pending.commands.lock().expect("poisoned").drain(..) { + for command in pending.commands.lock().drain(..) { let _result = handle.send(command); } @@ -3170,7 +3167,7 @@ impl Capture { slice.map_async(wgpu::MapMode::Read, { let map_result = map_result.clone(); move |result| { - *map_result.lock().assert("thread panicked") = Some(result); + *map_result.lock() = Some(result); } }); @@ -3179,7 +3176,7 @@ impl Capture { loop { device.poll(wgpu::Maintain::Poll); - let mut result = map_result.lock().assert("thread panicked"); + let mut result = map_result.lock(); if let Some(result) = result.take() { result?; break;