Remove Mutex from GdCellInner.

---------

- Post-CR fixes (to squash later).
- Properly lock cell state while dereferencing RefGuards.
- Safety comments.

Author: Yarwin

godot-cell/src/blocking_cell.rs

@@ -17,7 +17,7 @@ use crate::cell::GdCellInner;
 /// 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`.
+/// 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`].
 ///
@@ -168,10 +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> {}
+// 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)]

godot-cell/src/blocking_guards.rs

@@ -90,12 +90,14 @@ 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()
     }
 }
@@ -135,26 +137,24 @@ impl<'a, T> InaccessibleGuardBlocking<'a, T> {
     /// 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);
+    pub fn try_drop(self) -> Result<(), Self> {
         let can_drop = {
-            let _guard = manual.state.lock();
-            manual.inner.can_drop()
+            let _tracker_guard = self.state.lock();
+            self.inner.can_drop()
         };
 
         if !can_drop {
-            Err(manual)
+            Err(self)
         } else {
-            unsafe { ManuallyDrop::drop(&mut manual) };
             Ok(())
         }
     }
 }
 
 impl<T> Drop for InaccessibleGuardBlocking<'_, T> {
     fn drop(&mut self) {
-        let state_lock = self.state.lock().unwrap();
+        let _tracker_guard = self.state.lock().unwrap();
         unsafe { ManuallyDrop::drop(&mut self.inner) };
-        drop(state_lock)
+        drop(_tracker_guard)
     }
 }

godot-cell/src/cell.rs

@@ -89,35 +89,45 @@ pub(crate) struct GdCellInner<T> {
 impl<T> GdCellInner<T> {
     /// Creates a new cell storing `value`.
     pub fn new(value: T) -> Pin<Box<Self>> {
-        // Note – we are constructing it in two steps, because `CellState` uses non-null ptr to val inside our Cell.
         let mut uninitialized_cell: Box<MaybeUninit<Self>> = Box::new_uninit();
-        let ptr = uninitialized_cell.as_mut_ptr();
+        let uninitialized_cell_ptr = uninitialized_cell.as_mut_ptr();
 
-        unsafe {
-            (&raw mut (*ptr).value).write(UnsafeCell::new(value));
-        }
+        // 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: Box::pin(...) is equivalent to Box::into_pin(Box::...) therefore our freshly initialized
-        // `val` is and will stay valid (i.e. will refer to the same place after pinning).
+        // SAFETY: pointer to `state` is properly aligned.
+        let state_ptr = unsafe { &raw mut (*uninitialized_cell_ptr).state };
+
+        // SAFETY: See above.
         unsafe {
-            let val = (&raw const (*ptr).value).as_ref().unwrap();
-            (&raw mut (*ptr).state).write(UnsafeCell::new(CellState::new(val)));
+            state_ptr.write(UnsafeCell::new(CellState::new(value_ref)));
         }
 
-        Box::into_pin(unsafe { uninitialized_cell.assume_init() })
+        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 state = unsafe { &mut *self.state.get() };
-            state.borrow_state.increment_shared()?;
-        }
-
-        let state = unsafe { &*self.state.get() };
+        // 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, value)) }
@@ -127,7 +137,8 @@ impl<T> GdCellInner<T> {
     ///
     /// 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 state = unsafe { &mut *self.state.get() };
+        // 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();
@@ -159,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.
     ///
@@ -169,16 +199,15 @@ 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 = unsafe { &*self.state.get() };
-
+        // 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 = unsafe { &*self.state.get() };
-
-        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
     }
 }
 
@@ -206,8 +235,6 @@ 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(value: &UnsafeCell<T>) -> Self {
         Self {
             borrow_state: BorrowState::new(),
@@ -234,6 +261,17 @@ impl<T> CellState<T> {
         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)]

godot-cell/src/guards.rs

@@ -55,9 +55,8 @@ impl<T> Deref for RefGuard<'_, T> {
 
 impl<T> Drop for RefGuard<'_, T> {
     fn drop(&mut self) {
-        unsafe { self.state.get().as_mut() }
-            .unwrap()
-            .borrow_state
+        // SAFETY: There can't be any other active reference to CellState.
+        unsafe { CellState::borrow_state(self.state) }
             .decrement_shared()
             .unwrap();
     }
@@ -124,9 +123,8 @@ impl<T> Deref for MutGuard<'_, T> {
     type Target = T;
 
     fn deref(&self) -> &Self::Target {
-        let count = unsafe { self.state.get().as_ref().unwrap() }
-            .borrow_state
-            .mut_count();
+        // SAFETY: There can't be any other active reference to CellState.
+        let count = unsafe { CellState::borrow_state(self.state) }.mut_count();
         // This is just a best-effort error check. It should never be triggered.
         assert_eq!(
             self.count,
@@ -152,9 +150,8 @@ impl<T> Deref for MutGuard<'_, T> {
 
 impl<T> DerefMut for MutGuard<'_, T> {
     fn deref_mut(&mut self) -> &mut Self::Target {
-        let count = unsafe { self.state.get().as_ref().unwrap() }
-            .borrow_state
-            .mut_count();
+        // SAFETY: There can't be any other active reference to CellState.
+        let count = unsafe { CellState::borrow_state(self.state) }.mut_count();
         // This is just a best-effort error check. It should never be triggered.
         assert_eq!(
             self.count,
@@ -181,8 +178,7 @@ impl<T> DerefMut for MutGuard<'_, T> {
 
 impl<T> Drop for MutGuard<'_, T> {
     fn drop(&mut self) {
-        unsafe { self.state.get().as_mut().unwrap() }
-            .borrow_state
+        unsafe { CellState::borrow_state(self.state) }
             .decrement_mut()
             .unwrap();
     }
@@ -222,6 +218,7 @@ impl<'a, T> InaccessibleGuard<'a, T> {
     where
         'a: 'b,
     {
+        // SAFETY: There can be only one active reference to the cell state at a given time.
         let cell_state = unsafe { state.get().as_mut() }.unwrap();
         let current_ptr = cell_state.get_ptr();
         let new_ptr = NonNull::from(new_ref);
@@ -255,6 +252,11 @@ impl<'a, T> InaccessibleGuard<'a, T> {
         state.pop_ptr(prev_ptr);
     }
 
+    /// Returns `true` if guard can be safely dropped, i.e.:
+    ///
+    /// - Guard is being released in correct order.
+    /// - There is no accessible mutable reference to underlying value.
+    /// - There are no shared references to underlying value.
     #[doc(hidden)]
     pub fn can_drop(&self) -> bool {
         let state = unsafe { self.state.get().as_mut() }.unwrap();
@@ -266,11 +268,12 @@ impl<'a, T> InaccessibleGuard<'a, T> {
     /// 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);
-        if !manual.can_drop() {
-            return Err(manual);
+    pub fn try_drop(self) -> Result<(), Self> {
+        if !self.can_drop() {
+            return Err(self);
         }
+
+        let manual = std::mem::ManuallyDrop::new(self);
         Self::perform_drop(manual.state, manual.prev_ptr, manual.stack_depth);
 
         Ok(())

godot-cell/tests/mock/main.rs

@@ -135,7 +135,7 @@ macro_rules! setup_mock {
 
                 while let Some(guard) = guard_opt.take() {
                     if let Err(new_guard) = std::mem::ManuallyDrop::into_inner(guard).try_drop() {
-                        guard_opt = Some(new_guard);
+                        guard_opt = Some(std::mem::ManuallyDrop::new(new_guard));
                         std::hint::spin_loop()
                     }
                 }