Remove Mutex from GdCellInner.

Yarwin · Yarwin · commit 42dd9f4529f9 · 2025-12-15T19:16:29.000+01:00
---------

- Use `thread_tracker` to guard GdCellInner.
- Implement `InaccessibleGuardBlocking` which block thread upon dropping.
diff --git a/godot-cell/src/blocking_cell.rs b/godot-cell/src/blocking_cell.rs
@@ -11,13 +11,13 @@ use std::pin::Pin;
 use std::sync::{Arc, Condvar, Mutex, MutexGuard};
 use std::thread;
 
-use crate::blocking_guards::{MutGuardBlocking, RefGuardBlocking};
+use crate::blocking_guards::{InaccessibleGuardBlocking, MutGuardBlocking, RefGuardBlocking};
 use crate::cell::GdCellInner;
-use crate::guards::InaccessibleGuard;
 
 /// Blocking version of [`panicking::GdCell`](crate::panicking::GdCell) for multithreaded usage.
 ///
 /// This version of GdCell blocks the current thread if it does not yet hold references to the cell.
+/// Since `GdCellInner` isn't thread-safe by itself, any access to `inner` should be guarded by the `thread_tracker`.
 ///
 /// For more details on when threads are being blocked see [`Self::borrow`] and [`Self::borrow_mut`].
 ///
@@ -107,6 +107,7 @@ impl<T> GdCellBlocking<T> {
             inner_guard,
             self.mut_condition.clone(),
             self.immut_condition.clone(),
+            self.thread_tracker.clone(),
         ))
     }
 
@@ -119,11 +120,14 @@ impl<T> GdCellBlocking<T> {
     pub fn make_inaccessible<'cell, 'val>(
         &'cell self,
         current_ref: &'val mut T,
-    ) -> Result<InaccessibleGuard<'val, T>, Box<dyn Error>>
+    ) -> Result<InaccessibleGuardBlocking<'val, T>, Box<dyn Error>>
     where
         'cell: 'val,
     {
-        self.inner.as_ref().make_inaccessible(current_ref)
+        let _tracker_guard = self.thread_tracker.lock().unwrap();
+        let inner = self.inner.as_ref().make_inaccessible(current_ref)?;
+        let inaccessible = InaccessibleGuardBlocking::new(inner, self.thread_tracker.clone());
+        Ok(inaccessible)
     }
 
     /// Returns `true` if there are any mutable or shared references, regardless of whether the mutable
@@ -164,6 +168,11 @@ impl<T> GdCellBlocking<T> {
     }
 }
 
+// SAFETY: `T` is Sync, so we can return references to it on different threads.
+// It is also Send, so we can return mutable references to it on different threads.
+// Additionally, all internal state is synchronized via a mutex, so we won't have race conditions when trying to use it from multiple threads.
+unsafe impl<T: Send + Sync> Sync for GdCellBlocking<T> {}
+
 /// Holds the reference count and the currently mutable thread.
 #[derive(Debug)]
 pub(crate) struct ThreadTracker {
diff --git a/godot-cell/src/blocking_guards.rs b/godot-cell/src/blocking_guards.rs
@@ -11,6 +11,7 @@ use std::sync::{Arc, Condvar, Mutex};
 
 use crate::blocking_cell::ThreadTracker;
 use crate::guards::{MutGuard, RefGuard};
+use crate::panicking::InaccessibleGuard;
 
 /// Extended version of [`panicking::RefGuard`](crate::panicking::RefGuard) that tracks which thread a reference belongs to and when it's dropped.
 ///
@@ -50,7 +51,7 @@ impl<T> Drop for RefGuardBlocking<'_, T> {
 
         state_lock.decrement_current_thread_shared_count();
 
-        // SAFETY: guard is dropped exactly once, here.
+        // SAFETY: guard is dropped exactly once, here, while state is guarded by `_tracker_guard` preventing access from any other thread.
         unsafe { ManuallyDrop::drop(&mut self.inner) };
 
         self.mut_condition.notify_one();
@@ -66,18 +67,21 @@ pub struct MutGuardBlocking<'a, T> {
     inner: ManuallyDrop<MutGuard<'a, T>>,
     mut_condition: Arc<Condvar>,
     immut_condition: Arc<Condvar>,
+    state: Arc<Mutex<ThreadTracker>>,
 }
 
 impl<'a, T> MutGuardBlocking<'a, T> {
     pub(crate) fn new(
         inner: MutGuard<'a, T>,
         mut_condition: Arc<Condvar>,
         immut_condition: Arc<Condvar>,
+        state: Arc<Mutex<ThreadTracker>>,
     ) -> Self {
         Self {
             inner: ManuallyDrop::new(inner),
             immut_condition,
             mut_condition,
+            state,
         }
     }
 }
@@ -98,10 +102,59 @@ impl<T> DerefMut for MutGuardBlocking<'_, T> {
 
 impl<T> Drop for MutGuardBlocking<'_, T> {
     fn drop(&mut self) {
-        // SAFETY: guard is dropped exactly once, here.
+        let _tracker_guard = self.state.lock().unwrap();
+
+        // SAFETY: guard is dropped exactly once, here, while state is guarded by `_tracker_guard` preventing access from any other thread.
         unsafe { ManuallyDrop::drop(&mut self.inner) };
 
         self.mut_condition.notify_one();
         self.immut_condition.notify_all();
     }
 }
+
+/// Extended version of [`panicking::InaccessibleGuard`](crate::panicking::InaccessibleGuard) that blocks thread upon dropping.
+///
+/// See [`panicking::InaccessibleGuard`](crate::panicking::InaccessibleGuard) for more details.
+#[derive(Debug)]
+pub struct InaccessibleGuardBlocking<'a, T> {
+    inner: ManuallyDrop<InaccessibleGuard<'a, T>>,
+    state: Arc<Mutex<ThreadTracker>>,
+}
+
+impl<'a, T> InaccessibleGuardBlocking<'a, T> {
+    pub(crate) fn new(inner: InaccessibleGuard<'a, T>, state: Arc<Mutex<ThreadTracker>>) -> Self {
+        Self {
+            inner: ManuallyDrop::new(inner),
+            state,
+        }
+    }
+
+    /// Drop self if possible, otherwise returns self again.
+    ///
+    /// Used currently in the mock-tests, as we need a thread safe way to drop self. Using the normal drop
+    /// logic may poison state, however it should not cause any UB either way.
+    ///
+    /// See [`panicking::InaccessibleGuard::try_drop`](crate::panicking::InaccessibleGuard::try_drop) for more details.
+    pub fn try_drop(self) -> Result<(), ManuallyDrop<Self>> {
+        let mut manual = ManuallyDrop::new(self);
+        let can_drop = {
+            let _guard = manual.state.lock();
+            manual.inner.can_drop()
+        };
+
+        if !can_drop {
+            Err(manual)
+        } else {
+            unsafe { ManuallyDrop::drop(&mut manual) };
+            Ok(())
+        }
+    }
+}
+
+impl<T> Drop for InaccessibleGuardBlocking<'_, T> {
+    fn drop(&mut self) {
+        let state_lock = self.state.lock().unwrap();
+        unsafe { ManuallyDrop::drop(&mut self.inner) };
+        drop(state_lock)
+    }
+}
diff --git a/godot-cell/src/cell.rs b/godot-cell/src/cell.rs
@@ -73,7 +73,8 @@ impl<T> GdCell<T> {
 ///
 /// This cell must be pinned to be usable, as it stores self-referential pointers. The [`GdCell`] type abstracts this detail away from
 /// the public type.
-// TODO: consider not using `Mutex`
+///
+/// The cell is **not** thread-safe by itself.
 #[derive(Debug)]
 pub(crate) struct GdCellInner<T> {
     /// The mutable state of this cell.
@@ -181,13 +182,7 @@ impl<T> GdCellInner<T> {
     }
 }
 
-// SAFETY: `T` is Sync, so we can return references to it on different threads.
-// It is also Send, so we can return mutable references to it on different threads.
-// Additionally, all internal state is synchronized via a mutex, so we won't have race conditions when trying to use it from multiple threads.
-unsafe impl<T: Send + Sync> Sync for GdCellInner<T> {}
-
-/// Mutable state of the `GdCell`, bundled together to make it easier to avoid deadlocks when locking the
-/// mutex.
+/// Mutable state of the `GdCell`.
 #[derive(Debug)]
 pub(crate) struct CellState<T> {
     /// Tracking the borrows this cell has. This ensures relevant invariants are upheld.
diff --git a/godot-cell/src/guards.rs b/godot-cell/src/guards.rs
@@ -255,15 +255,20 @@ impl<'a, T> InaccessibleGuard<'a, T> {
         state.pop_ptr(prev_ptr);
     }
 
+    #[doc(hidden)]
+    pub fn can_drop(&self) -> bool {
+        let state = unsafe { self.state.get().as_mut() }.unwrap();
+        state.borrow_state.may_unset_inaccessible() || state.stack_depth == self.stack_depth
+    }
+
     /// Drop self if possible, otherwise returns self again.
     ///
     /// Used currently in the mock-tests, as we need a thread safe way to drop self. Using the normal drop
     /// logic may poison state, however it should not cause any UB either way.
     #[doc(hidden)]
     pub fn try_drop(self) -> Result<(), std::mem::ManuallyDrop<Self>> {
         let manual = std::mem::ManuallyDrop::new(self);
-        let state = unsafe { manual.state.get().as_mut() }.unwrap();
-        if !state.borrow_state.may_unset_inaccessible() || state.stack_depth != manual.stack_depth {
+        if !manual.can_drop() {
             return Err(manual);
         }
         Self::perform_drop(manual.state, manual.prev_ptr, manual.stack_depth);
diff --git a/godot-cell/src/lib.rs b/godot-cell/src/lib.rs
@@ -43,6 +43,8 @@ pub mod panicking {
 
 pub mod blocking {
     pub use crate::blocking_cell::GdCellBlocking as GdCell;
-    pub use crate::blocking_guards::{MutGuardBlocking as MutGuard, RefGuardBlocking as RefGuard};
-    pub use crate::guards::InaccessibleGuard;
+    pub use crate::blocking_guards::{
+        InaccessibleGuardBlocking as InaccessibleGuard, MutGuardBlocking as MutGuard,
+        RefGuardBlocking as RefGuard,
+    };
 }
diff --git a/godot-cell/tests/mock/panicking.rs b/godot-cell/tests/mock/panicking.rs
@@ -8,6 +8,9 @@
 //! A mock implementation of our instance-binding pattern in pure rust.
 //!
 //! Used so we can run miri on this, which we cannot when we are running in itest against Godot.
+//!
+//! Currently, the panicking `GdCell` is suitable only for single-threaded use. Without `experimental-threads` enabled,
+//! godot-rust will block access to bindings from any thread other than the main one.
 
 use std::collections::HashMap;
 use std::error::Error;
@@ -55,131 +58,3 @@ fn all_calls_work() {
         }
     }
 }
-
-/// Run each method both from the main thread and a newly created thread.
-#[test]
-fn calls_different_thread() {
-    use std::thread;
-
-    let instance_id = MyClass::init();
-
-    // We're not running in parallel, so it will never fail to increment completely.
-    for (f, _, expected_increment) in CALLS {
-        let start = unsafe { get_int(instance_id) };
-        unsafe {
-            f(instance_id).unwrap();
-
-            assert_id_is(instance_id, start + expected_increment);
-        }
-        let start = start + expected_increment;
-        thread::spawn(move || unsafe { f(instance_id).unwrap() })
-            .join()
-            .unwrap();
-        unsafe {
-            assert_id_is(instance_id, start + expected_increment);
-        }
-    }
-}
-
-/// Call each method from different threads, allowing them to run in parallel.
-///
-/// This may cause borrow failures, we do a best-effort attempt at estimating the value then. We can detect
-/// if the first call failed, so then we know the integer was incremented by 0. Otherwise, we at least know
-/// the range of values that it can be incremented by.
-#[test]
-fn calls_parallel() {
-    use std::thread;
-
-    let instance_id = MyClass::init();
-    let mut handles = Vec::new();
-
-    for (f, min_increment, max_increment) in CALLS {
-        let handle = thread::spawn(move || unsafe {
-            f(instance_id).map_or((0, 0), |_| (*min_increment, *max_increment))
-        });
-        handles.push(handle);
-    }
-
-    let (min_expected, max_expected) = handles
-        .into_iter()
-        .map(|handle| handle.join().unwrap())
-        .reduce(|(curr_min, curr_max), (min, max)| (curr_min + min, curr_max + max))
-        .unwrap();
-
-    unsafe {
-        assert!(get_int(instance_id) >= min_expected);
-        assert!(get_int(instance_id) <= max_expected);
-    }
-}
-
-/// Call each method from different threads, allowing them to run in parallel.
-///
-/// This may cause borrow failures, we do a best-effort attempt at estimating the value then. We can detect
-/// if the first call failed, so then we know the integer was incremented by 0. Otherwise, we at least know
-/// the range of values that it can be incremented by.
-///
-/// Runs each method several times in a row. This should reduce the non-determinism that comes from
-/// scheduling of threads.
-#[test]
-fn calls_parallel_many_serial() {
-    use std::thread;
-
-    let instance_id = MyClass::init();
-    let mut handles = Vec::new();
-
-    for (f, min_increment, max_increment) in CALLS {
-        for _ in 0..10 {
-            let handle = thread::spawn(move || unsafe {
-                f(instance_id).map_or((0, 0), |_| (*min_increment, *max_increment))
-            });
-            handles.push(handle);
-        }
-    }
-
-    let (min_expected, max_expected) = handles
-        .into_iter()
-        .map(|handle| handle.join().unwrap())
-        .reduce(|(curr_min, curr_max), (min, max)| (curr_min + min, curr_max + max))
-        .unwrap();
-
-    unsafe {
-        assert!(get_int(instance_id) >= min_expected);
-        assert!(get_int(instance_id) <= max_expected);
-    }
-}
-
-/// Call each method from different threads, allowing them to run in parallel.
-///
-/// This may cause borrow failures, we do a best-effort attempt at estimating the value then. We can detect
-/// if the first call failed, so then we know the integer was incremented by 0. Otherwise, we at least know
-/// the range of values that it can be incremented by.
-///
-/// Runs all the tests several times. This is different from [`calls_parallel_many_serial`] as that calls the
-/// methods like AAA...BBB...CCC..., whereas this interleaves the methods like ABC...ABC...ABC...
-#[test]
-fn calls_parallel_many_parallel() {
-    use std::thread;
-
-    let instance_id = MyClass::init();
-    let mut handles = Vec::new();
-
-    for _ in 0..10 {
-        for (f, min_increment, max_increment) in CALLS {
-            let handle = thread::spawn(move || unsafe {
-                f(instance_id).map_or((0, 0), |_| (*min_increment, *max_increment))
-            });
-            handles.push(handle);
-        }
-    }
-
-    let (min_expected, max_expected) = handles
-        .into_iter()
-        .map(|handle| handle.join().unwrap())
-        .reduce(|(curr_min, curr_max), (min, max)| (curr_min + min, curr_max + max))
-        .unwrap();
-
-    unsafe {
-        assert!(get_int(instance_id) >= min_expected);
-        assert!(get_int(instance_id) <= max_expected);
-    }
-}
