GdCell::borrow_mut should block on main thread if shared ref exists

godot-cell/src/blocking_cell.rs

@@ -54,10 +54,27 @@ impl<T> GdCellBlocking<T> {
             tracker_guard = self.block_immut(tracker_guard);
         }
 
+        let should_claim_mut = !self.is_currently_bound();
+
         let inner_guard = self.inner.as_ref().borrow()?;
 
         tracker_guard.increment_current_thread_shared_count();
 
+        // The ThreadTracker::mut_thread is always set to some ThreadId to avoid additional tracking overhead, but this causes an edge case:
+        // 1. mut_thread is initialized with the current Thread 1 (usually main thread).
+        // 2. Thread 2 acquires an immutable borrow from the cell.
+        // 3. Thread 1 attempts to acquire a mutable borrow from the cell.
+        // 4. No immutable borrow exists on Thread 1, but the thread is assigned as the mut_thread; no blocking occurs.
+        // 5. The mutable borrow fails and panics because there is already an immutable borrow on Thread 1.
+        //
+        // Solution:
+        // 1. Always reassign mut_thread to the current ThreadId (i.e. Thread 2) when the first immutable borrow is acquired.
+        // 2. Thread 1 will now block on mutable borrows because it is not the mut_thread.
+        // 3. Thread 2 should never block, as it already holds an immutable borrow.
+        if should_claim_mut {
+            tracker_guard.claim_mut_ref();
+        }
+
         Ok(RefGuardBlocking::new(
             inner_guard,
             self.mut_condition.clone(),
@@ -84,7 +101,7 @@ impl<T> GdCellBlocking<T> {
 
         let inner_guard = self.inner.as_ref().borrow_mut()?;
 
-        tracker_guard.mut_thread = thread::current().id();
+        tracker_guard.claim_mut_ref();
 
         Ok(MutGuardBlocking::new(
             inner_guard,
@@ -151,6 +168,10 @@ impl<T> GdCellBlocking<T> {
 #[derive(Debug)]
 pub(crate) struct ThreadTracker {
     /// Thread ID of the thread that currently can hold the mutable reference.
+    ///
+    /// This is not an Option, contrary to what one might expect. Making this an option would require reliable knowledge that not a single
+    /// MutGuardBlocking exists before setting it to None. This would require tracking a count of mutable borrows. Instead, we always set
+    /// the field to an acceptable value and avoid the overhead.
     mut_thread: thread::ThreadId,
 
     /// Shared reference count per thread.
@@ -204,4 +225,9 @@ impl ThreadTracker {
     pub fn current_thread_has_mut_ref(&self) -> bool {
         self.mut_thread == thread::current().id()
     }
+
+    /// Claims the mutable reference for the current thread.
+    fn claim_mut_ref(&mut self) {
+        self.mut_thread = thread::current().id();
+    }
 }

godot-cell/tests/mock/blocking.rs

@@ -13,6 +13,7 @@ use std::collections::HashMap;
 use std::error::Error;
 use std::marker::PhantomData;
 use std::sync::{atomic::AtomicUsize, Mutex, OnceLock};
+use std::time::Duration;
 
 use godot_cell::blocking::{GdCell, InaccessibleGuard};
 
@@ -26,6 +27,10 @@ impl MyClass {
     fn immut_calls_mut_directly(&self) {
         unsafe { call_mut_method(self.base.instance_id, Self::mut_method).unwrap() }
     }
+
+    fn immut_with_sleep(&self) {
+        std::thread::sleep(Duration::from_millis(100));
+    }
 }
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
@@ -160,3 +165,25 @@ fn non_blocking_reborrow() {
 
     assert_eq!(panic_b.unwrap().downcast_ref::<String>().unwrap(), "called `Result::unwrap()` on an `Err` value: Custom(\"cannot borrow mutable while shared borrow exists\")");
 }
+
+/// Mutable borrow on main thread with shared borrow on others.
+///
+/// This verifies that the thread which initialized the `GdCell` does not panic when it attempts to mutably borrow while there is already a
+/// shared borrow on an other thread.
+#[test]
+fn no_mut_panic_on_main() {
+    use std::thread;
+    let instance_id = MyClass::init();
+
+    let thread_a = thread::spawn(move || unsafe {
+        // Acquire an immutable reference and sleep for a while.
+        call_immut_method(instance_id, MyClass::immut_with_sleep).unwrap();
+    });
+
+    thread::sleep(Duration::from_millis(50));
+
+    let main_result = unsafe { call_mut_method(instance_id, MyClass::mut_method) };
+
+    main_result.expect("The main thread should not panic!");
+    thread_a.join().expect("Thread_a should not panic!");
+}