Add multi-threaded reentrant

.github/workflows/full-ci.yml

@@ -320,6 +320,12 @@ jobs:
             godot-binary: godot.linuxbsd.editor.dev.x86_64
             rust-extra-args: --features godot/custom-godot,godot/experimental-threads,godot/serde
 
+          - name: linux-features-bind-self
+            os: ubuntu-20.04
+            artifact-name: linux-nightly
+            godot-binary: godot.linuxbsd.editor.dev.x86_64
+            rust-extra-args: --features godot/custom-godot,godot/experimental-threads,godot/serde,itest/experimental-bind-self
+
           # TODO merge with other jobs
           - name: linux-lazy-fptrs
             os: ubuntu-20.04

godot-cell/src/lib.rs

@@ -120,6 +120,8 @@ impl<T> GdCell<T> {
     }
 }
 
+unsafe impl<T: Sync> Sync for GdCell<T> {}
+
 #[cfg(test)]
 mod test {
     use std::pin::pin;

godot-cell/tests/mock.rs

@@ -237,3 +237,78 @@ fn all_calls_work() {
     unsafe { call_immut_method(instance_id, MyClass::immut_calls_immut_directly).unwrap() };
     assert_int_is(instance_id, 9);
 }
+
+#[test]
+fn calls_different_thread() {
+    use std::thread;
+
+    let instance_id = MyClass::init();
+    fn assert_int_is(instance_id: usize, target: i64) {
+        let storage = unsafe { get_instance::<MyClass>(instance_id) };
+        let bind = storage.cell.as_ref().borrow().unwrap();
+        assert_eq!(bind.int, target);
+    }
+
+    assert_int_is(instance_id, 0);
+
+    unsafe { call_immut_method(instance_id, MyClass::immut_method).unwrap() };
+    assert_int_is(instance_id, 0);
+    thread::spawn(move || unsafe {
+        call_immut_method(instance_id, MyClass::immut_method).unwrap()
+    })
+    .join()
+    .unwrap();
+    assert_int_is(instance_id, 0);
+
+    unsafe { call_mut_method(instance_id, MyClass::mut_method).unwrap() };
+    assert_int_is(instance_id, 1);
+    thread::spawn(move || unsafe { call_mut_method(instance_id, MyClass::mut_method).unwrap() })
+        .join()
+        .unwrap();
+    assert_int_is(instance_id, 2);
+
+    unsafe { call_mut_method(instance_id, MyClass::mut_method_calls_immut).unwrap() };
+    assert_int_is(instance_id, 3);
+    thread::spawn(move || unsafe {
+        call_mut_method(instance_id, MyClass::mut_method_calls_immut).unwrap()
+    })
+    .join()
+    .unwrap();
+    assert_int_is(instance_id, 4);
+
+    unsafe { call_mut_method(instance_id, MyClass::mut_method_calls_mut).unwrap() };
+    assert_int_is(instance_id, 6);
+    thread::spawn(move || unsafe {
+        call_mut_method(instance_id, MyClass::mut_method_calls_mut).unwrap()
+    })
+    .join()
+    .unwrap();
+    assert_int_is(instance_id, 8);
+
+    unsafe { call_mut_method(instance_id, MyClass::mut_method_calls_twice).unwrap() };
+    assert_int_is(instance_id, 10);
+    thread::spawn(move || unsafe {
+        call_mut_method(instance_id, MyClass::mut_method_calls_twice).unwrap()
+    })
+    .join()
+    .unwrap();
+    assert_int_is(instance_id, 12);
+
+    unsafe { call_mut_method(instance_id, MyClass::mut_method_calls_twice_mut).unwrap() };
+    assert_int_is(instance_id, 15);
+    thread::spawn(move || unsafe {
+        call_mut_method(instance_id, MyClass::mut_method_calls_twice_mut).unwrap()
+    })
+    .join()
+    .unwrap();
+    assert_int_is(instance_id, 18);
+
+    unsafe { call_immut_method(instance_id, MyClass::immut_calls_immut_directly).unwrap() };
+    assert_int_is(instance_id, 18);
+    thread::spawn(move || unsafe {
+        call_immut_method(instance_id, MyClass::immut_calls_immut_directly).unwrap()
+    })
+    .join()
+    .unwrap();
+    assert_int_is(instance_id, 18);
+}

godot-core/src/storage.rs

@@ -17,6 +17,38 @@ pub enum Lifecycle {
     Destroying,
 }
 
+#[cfg_attr(not(feature = "experimental-threads"), allow(dead_code))]
+pub struct AtomicLifecycle {
+    atomic: std::sync::atomic::AtomicU32,
+}
+
+#[cfg_attr(not(feature = "experimental-threads"), allow(dead_code))]
+impl AtomicLifecycle {
+    pub fn new(value: Lifecycle) -> Self {
+        Self {
+            atomic: std::sync::atomic::AtomicU32::new(value as u32),
+        }
+    }
+
+    pub fn get(&self) -> Lifecycle {
+        match self.atomic.load(std::sync::atomic::Ordering::Relaxed) {
+            0 => Lifecycle::Alive,
+            1 => Lifecycle::Destroying,
+            other => panic!("invalid lifecycle {other}"),
+        }
+    }
+
+    pub fn set(&self, lifecycle: Lifecycle) {
+        let value = match lifecycle {
+            Lifecycle::Alive => 0,
+            Lifecycle::Destroying => 1,
+        };
+
+        self.atomic
+            .store(value, std::sync::atomic::Ordering::Relaxed);
+    }
+}
+
 #[cfg_attr(
     all(
         not(feature = "experimental-bind-self"),
@@ -36,7 +68,11 @@ mod single_threaded;
 mod multi_threaded;
 
 #[cfg(feature = "experimental-bind-self")]
-mod reentrant;
+#[cfg_attr(not(feature = "experimental-threads"), allow(dead_code))]
+mod reentrant_multi_threaded;
+#[cfg(feature = "experimental-bind-self")]
+#[cfg_attr(feature = "experimental-threads", allow(dead_code))]
+mod reentrant_single_threaded;
 
 /// A storage for an instance binding.
 ///
@@ -171,13 +207,21 @@ pub(crate) trait StorageRefCounted: Storage {
     not(feature = "experimental-bind-self")
 ))]
 pub type InstanceStorage<T> = single_threaded::InstanceStorage<T>;
+
 #[cfg(all(
     feature = "experimental-threads",
     not(feature = "experimental-bind-self")
 ))]
 pub type InstanceStorage<T> = multi_threaded::InstanceStorage<T>;
-#[cfg(feature = "experimental-bind-self")]
-pub type InstanceStorage<T> = reentrant::InstanceStorage<T>;
+
+#[cfg(all(
+    not(feature = "experimental-threads"),
+    feature = "experimental-bind-self"
+))]
+pub type InstanceStorage<T> = reentrant_single_threaded::InstanceStorage<T>;
+
+#[cfg(all(feature = "experimental-threads", feature = "experimental-bind-self"))]
+pub type InstanceStorage<T> = reentrant_multi_threaded::InstanceStorage<T>;
 
 pub type RefGuard<'a, T> = <InstanceStorage<T> as Storage>::RefGuard<'a>;
 pub type MutGuard<'a, T> = <InstanceStorage<T> as Storage>::MutGuard<'a>;

godot-core/src/storage/multi_threaded.rs

@@ -10,36 +10,7 @@ use std::sync::atomic::{AtomicU32, Ordering};
 use crate::obj::{Base, GodotClass};
 use crate::out;
 
-use super::{Lifecycle, Storage, StorageRefCounted};
-
-pub struct AtomicLifecycle {
-    atomic: AtomicU32,
-}
-
-impl AtomicLifecycle {
-    pub fn new(value: Lifecycle) -> Self {
-        Self {
-            atomic: AtomicU32::new(value as u32),
-        }
-    }
-
-    pub fn get(&self) -> Lifecycle {
-        match self.atomic.load(Ordering::Relaxed) {
-            0 => Lifecycle::Alive,
-            1 => Lifecycle::Destroying,
-            other => panic!("invalid lifecycle {other}"),
-        }
-    }
-
-    pub fn set(&self, lifecycle: Lifecycle) {
-        let value = match lifecycle {
-            Lifecycle::Alive => 0,
-            Lifecycle::Destroying => 1,
-        };
-
-        self.atomic.store(value, Ordering::Relaxed);
-    }
-}
+use super::{AtomicLifecycle, Lifecycle, Storage, StorageRefCounted};
 
 /// Manages storage and lifecycle of user's extension class instances.
 pub struct InstanceStorage<T: GodotClass> {

godot-core/src/storage/reentrant_multi_threaded.rs

@@ -0,0 +1,150 @@
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/.
+ */
+
+use std::any::type_name;
+use std::pin::Pin;
+use std::sync::atomic::{AtomicU32, Ordering};
+
+use crate::obj::{Base, GodotClass};
+use crate::out;
+
+use super::{AtomicLifecycle, Lifecycle, Storage, StorageRefCounted};
+
+pub struct InstanceStorage<T: GodotClass> {
+    user_instance: Pin<Box<godot_cell::GdCell<T>>>,
+    pub(super) base: Base<T::Base>,
+
+    // Declared after `user_instance`, is dropped last
+    pub(super) lifecycle: AtomicLifecycle,
+    godot_ref_count: AtomicU32,
+}
+
+// SAFETY:
+// The only way to get a reference to the user instance is by going through the `GdCell` in `user_instance`.
+// If this `GdCell` has returned any references, then `self.user_instance.as_ref().is_currently_bound()` will
+// return true. So `is_bound` will return true when a reference to the user instance exists.
+//
+// If `is_bound` is false, then there are no references to the user instance in this storage. And no other
+// references to data in this storage can exist. So we can safely drop it.
+unsafe impl<T: GodotClass> Storage for InstanceStorage<T> {
+    type Instance = T;
+
+    type RefGuard<'a> = godot_cell::RefGuard<'a, T>;
+
+    type MutGuard<'a> = godot_cell::MutGuard<'a, T>;
+
+    type BaseMutGuard<'a> = godot_cell::NonAliasingGuard<'a, T>;
+
+    fn construct(
+        user_instance: Self::Instance,
+        base: Base<<Self::Instance as GodotClass>::Base>,
+    ) -> Self {
+        out!("    Storage::construct             <{}>", type_name::<T>());
+        Self {
+            user_instance: Box::pin(godot_cell::GdCell::new(user_instance)),
+            base,
+            lifecycle: AtomicLifecycle::new(Lifecycle::Alive),
+            godot_ref_count: AtomicU32::new(1),
+        }
+    }
+
+    fn is_bound(&self) -> bool {
+        self.user_instance.as_ref().is_currently_bound()
+    }
+
+    fn base(&self) -> &Base<<Self::Instance as GodotClass>::Base> {
+        &self.base
+    }
+
+    fn get(&self) -> Self::RefGuard<'_> {
+        self.user_instance.as_ref().borrow().unwrap_or_else(|err| {
+            panic!(
+                "\
+                    Gd<T>::bind() failed, already bound; T = {}.\n  \
+                    Make sure to use `self.base_mut()` or `self.base()` instead of `self.to_gd()` when possible.\n  \
+                    Details: {err}.\
+                ",
+                type_name::<T>()
+            )
+        })
+    }
+
+    fn get_mut(&self) -> Self::MutGuard<'_> {
+        self.user_instance
+            .as_ref()
+            .borrow_mut()
+            .unwrap_or_else(|err| {
+                panic!(
+                    "\
+                    Gd<T>::bind_mut() failed, already bound; T = {}.\n  \
+                    Make sure to use `self.base_mut()` instead of `self.to_gd()` when possible.\n  \
+                    Details: {err}.\
+                ",
+                    type_name::<T>()
+                )
+            })
+    }
+
+    fn get_base_mut<'a: 'b, 'b>(&'a self, value: &'b mut Self::Instance) -> Self::BaseMutGuard<'b> {
+        self.user_instance
+            .as_ref()
+            .set_non_aliasing(value)
+            .unwrap_or_else(|err| {
+                // We should never hit this, except maybe in extreme cases like having more than
+                // `usize::MAX` borrows.
+                panic!(
+                    "\
+                        `base_mut()` failed for type T = {}.\n  \
+                        This is most likely a bug, please report it.\n  \
+                        Details: {err}.\
+                    ",
+                    type_name::<T>()
+                )
+            })
+    }
+
+    fn get_lifecycle(&self) -> Lifecycle {
+        self.lifecycle.get()
+    }
+
+    fn set_lifecycle(&self, lifecycle: Lifecycle) {
+        self.lifecycle.set(lifecycle)
+    }
+}
+
+impl<T: GodotClass> StorageRefCounted for InstanceStorage<T> {
+    fn godot_ref_count(&self) -> u32 {
+        self.godot_ref_count.load(Ordering::Relaxed)
+    }
+
+    fn on_inc_ref(&self) {
+        self.godot_ref_count.fetch_add(1, Ordering::Relaxed);
+        out!(
+            "    Storage::on_inc_ref (rc={})     <{:?}>",
+            self.godot_ref_count(),
+            self.base,
+        );
+    }
+
+    fn on_dec_ref(&self) {
+        self.godot_ref_count.fetch_sub(1, Ordering::Relaxed);
+        out!(
+            "  | Storage::on_dec_ref (rc={})     <{:?}>",
+            self.godot_ref_count(),
+            self.base,
+        );
+    }
+}
+
+impl<T: GodotClass> Drop for InstanceStorage<T> {
+    fn drop(&mut self) {
+        out!(
+            "    Storage::drop (rc={})           <{:?}>",
+            self.godot_ref_count(),
+            self.base(),
+        );
+    }
+}