Remove Mutex from GdCellInner

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` must be guarded by locking 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,12 @@ impl<T> GdCellBlocking<T> {
     }
 }
 
+// SAFETY:
+// - `T` must not be `Sync`, and the only way to access the underlying `T` is via `GdCellBlocking`.
+// - It must be ensured that `GdCellInner`, which holds `T`, cannot be accessed from multiple threads simultaneously while handing out guards.
+// The current implementation ensures this by locking the `thread_tracker`.
+unsafe impl<T: Send> Sync for GdCellBlocking<T> {}
+
 /// Holds the reference count and the currently mutable thread.
 #[derive(Debug)]
 pub(crate) struct ThreadTracker {

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,
         }
     }
 }
@@ -86,22 +90,71 @@ impl<'a, T> Deref for MutGuardBlocking<'a, T> {
     type Target = <MutGuard<'a, T> as Deref>::Target;
 
     fn deref(&self) -> &Self::Target {
+        let _tracker_guard = self.state.lock().unwrap();
         self.inner.deref().deref()
     }
 }
 
 impl<T> DerefMut for MutGuardBlocking<'_, T> {
     fn deref_mut(&mut self) -> &mut Self::Target {
+        let _tracker_guard = self.state.lock().unwrap();
         self.inner.deref_mut().deref_mut()
     }
 }
 
 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<(), Self> {
+        let can_drop = {
+            let _tracker_guard = self.state.lock();
+            self.inner.can_drop()
+        };
+
+        if !can_drop {
+            Err(self)
+        } else {
+            Ok(())
+        }
+    }
+}
+
+impl<T> Drop for InaccessibleGuardBlocking<'_, T> {
+    fn drop(&mut self) {
+        let _tracker_guard = self.state.lock().unwrap();
+        unsafe { ManuallyDrop::drop(&mut self.inner) };
+        drop(_tracker_guard)
+    }
+}

godot-cell/src/cell.rs

@@ -8,9 +8,9 @@
 use std::cell::UnsafeCell;
 use std::error::Error;
 use std::marker::PhantomPinned;
+use std::mem::MaybeUninit;
 use std::pin::Pin;
 use std::ptr::NonNull;
-use std::sync::Mutex;
 
 use crate::borrow_state::BorrowState;
 use crate::guards::{InaccessibleGuard, MutGuard, RefGuard};
@@ -73,11 +73,12 @@ 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.
-    pub(crate) state: Mutex<CellState<T>>,
+    pub(crate) state: UnsafeCell<CellState<T>>,
     /// The actual value we're handing out references to, uses `UnsafeCell` as we're passing out `&mut`
     /// references to its contents even when we only have a `&` reference to the cell.
     value: UnsafeCell<T>,
@@ -88,34 +89,56 @@ pub(crate) struct GdCellInner<T> {
 impl<T> GdCellInner<T> {
     /// Creates a new cell storing `value`.
     pub fn new(value: T) -> Pin<Box<Self>> {
-        let cell = Box::pin(Self {
-            state: Mutex::new(CellState::new()),
-            value: UnsafeCell::new(value),
-            _pin: PhantomPinned,
-        });
-
-        cell.state.lock().unwrap().initialize_ptr(&cell.value);
+        let mut uninitialized_cell: Box<MaybeUninit<Self>> = Box::new_uninit();
+        let uninitialized_cell_ptr = uninitialized_cell.as_mut_ptr();
+
+        // SAFETY: pointer to `value` is properly aligned.
+        let value_ptr = unsafe {
+            let value_ptr = &raw mut (*uninitialized_cell_ptr).value;
+            value_ptr.write(UnsafeCell::new(value));
+            value_ptr
+        };
+
+        // SAFETY
+        // `value_ptr` is properly aligned and points to initialized data.
+        // Additionally, since Box::pin(...) is equivalent to Box::into_pin(Box::...) `value_ref`
+        // will remain valid and refer to the same underlying value after pinning.
+        let value_ref = unsafe { value_ptr.as_ref().unwrap() };
+
+        // SAFETY: pointer to `state` is properly aligned.
+        let state_ptr = unsafe { &raw mut (*uninitialized_cell_ptr).state };
+
+        // SAFETY: See above.
+        unsafe {
+            state_ptr.write(UnsafeCell::new(CellState::new(value_ref)));
+        }
 
-        cell
+        Box::into_pin(
+            // SAFETY: All `GdCellInner` fields are valid.
+            unsafe { uninitialized_cell.assume_init() },
+        )
     }
 
     /// Returns a new shared reference to the contents of the cell.
     ///
     /// Fails if an accessible mutable reference exists.
     pub fn borrow(self: Pin<&Self>) -> Result<RefGuard<'_, T>, Box<dyn Error>> {
-        let mut state = self.state.lock().unwrap();
+        // SAFETY: This is the only active reference to the state.
+        let state = unsafe { self.cell_state_mut() };
         state.borrow_state.increment_shared()?;
+        let value = state.get_ptr();
 
         // SAFETY: `increment_shared` succeeded, therefore there cannot currently be any accessible mutable
         // references.
-        unsafe { Ok(RefGuard::new(&self.get_ref().state, state.get_ptr())) }
+        unsafe { Ok(RefGuard::new(&self.get_ref().state, value)) }
     }
 
     /// Returns a new mutable reference to the contents of the cell.
     ///
     /// Fails if an accessible mutable reference exists, or a shared reference exists.
     pub fn borrow_mut(self: Pin<&Self>) -> Result<MutGuard<'_, T>, Box<dyn Error>> {
-        let mut state = self.state.lock().unwrap();
+        // SAFETY: This is the only active reference to the state.
+        let state = unsafe { self.cell_state_mut() };
         state.borrow_state.increment_mut()?;
         let count = state.borrow_state.mut_count();
         let value = state.get_ptr();
@@ -147,6 +170,25 @@ impl<T> GdCellInner<T> {
         InaccessibleGuard::new(&self.get_ref().state, current_ref)
     }
 
+    /// Returns a reference to the CellState.
+    ///
+    /// # Safety
+    /// - The caller must ensure that there are no active exclusive references to the given state.
+    unsafe fn cell_state(&self) -> &CellState<T> {
+        // SAFETY: the underlying `CellState` will not be deallocated as long as Cell itself is alive.
+        unsafe { &*self.state.get() }
+    }
+
+    /// Returns the exclusive reference to the CellState.
+    ///
+    /// # Safety
+    /// - The caller must ensure that there are no active references to the given state.
+    #[allow(clippy::mut_from_ref)]
+    unsafe fn cell_state_mut(&self) -> &mut CellState<T> {
+        // SAFETY: the underlying `CellState` will not be deallocated as long as Cell itself is alive.
+        unsafe { &mut *self.state.get() }
+    }
+
     /// Returns `true` if there are any mutable or shared references, regardless of whether the mutable
     /// references are accessible or not.
     ///
@@ -157,26 +199,19 @@ impl<T> GdCellInner<T> {
     /// cell hands out a new borrow before it is destroyed. So we still need to ensure that this cannot
     /// happen at the same time.
     pub fn is_currently_bound(self: Pin<&Self>) -> bool {
-        let state = self.state.lock().unwrap();
-
+        // SAFETY: this is the only reference to the `cell_state` in given context.
+        let state = unsafe { self.cell_state() };
         state.borrow_state.shared_count() > 0 || state.borrow_state.mut_count() > 0
     }
 
     /// Similar to [`Self::is_currently_bound`] but only counts mutable references and ignores shared references.
     pub(crate) fn is_currently_mutably_bound(self: Pin<&Self>) -> bool {
-        let state = self.state.lock().unwrap();
-
-        state.borrow_state.mut_count() > 0
+        // SAFETY: this is the only reference to the `cell_state` in given context.
+        unsafe { self.cell_state() }.borrow_state.mut_count() > 0
     }
 }
 
-// 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.
@@ -191,8 +226,7 @@ pub(crate) struct CellState<T> {
     ///
     /// We always generate new pointer based off of the pointer currently in this field, to ensure any new
     /// references are derived from the most recent `&mut` reference.
-    // TODO: Consider using `NonNull<T>` instead.
-    ptr: *mut T,
+    ptr: NonNull<T>,
 
     /// How many pointers have been handed out.
     ///
@@ -201,44 +235,43 @@ pub(crate) struct CellState<T> {
 }
 
 impl<T> CellState<T> {
-    /// Create a new uninitialized state. Use [`initialize_ptr()`](CellState::initialize_ptr()) to initialize
-    /// it.
-    fn new() -> Self {
+    fn new(value: &UnsafeCell<T>) -> Self {
         Self {
             borrow_state: BorrowState::new(),
-            ptr: std::ptr::null_mut(),
+            ptr: NonNull::new(value.get()).unwrap(),
             stack_depth: 0,
         }
     }
 
-    /// Initialize the pointer if it is `None`.
-    fn initialize_ptr(&mut self, value: &UnsafeCell<T>) {
-        if self.ptr.is_null() {
-            self.ptr = value.get();
-            assert!(!self.ptr.is_null());
-        } else {
-            panic!("Cannot initialize pointer as it is already initialized.")
-        }
-    }
-
     /// Returns the current pointer. Panics if uninitialized.
     pub(crate) fn get_ptr(&self) -> NonNull<T> {
-        NonNull::new(self.ptr).unwrap()
+        self.ptr
     }
 
     /// Push a pointer to this state.
     pub(crate) fn push_ptr(&mut self, new_ptr: NonNull<T>) -> usize {
-        self.ptr = new_ptr.as_ptr();
+        self.ptr = new_ptr;
         self.stack_depth += 1;
         self.stack_depth
     }
 
     /// Pop a pointer to this state, resetting it to the given old pointer.
     pub(crate) fn pop_ptr(&mut self, old_ptr: NonNull<T>) -> usize {
-        self.ptr = old_ptr.as_ptr();
+        self.ptr = old_ptr;
         self.stack_depth -= 1;
         self.stack_depth
     }
+
+    /// Returns underlying [`BorrowState`].
+    ///
+    /// # Safety
+    ///
+    /// - `cell_state` must point to a valid reference.
+    /// - There can't be any active reference to `CellState`.
+    #[allow(clippy::mut_from_ref)]
+    pub(crate) unsafe fn borrow_state(cell_state: &UnsafeCell<Self>) -> &mut BorrowState {
+        &mut cell_state.get().as_mut().unwrap().borrow_state
+    }
 }
 
 #[cfg(test)]