Merge #212

212: Implement InstanceStorage optionally as sync for #18 r=Bromeon a=TitanNano

This is a second implementation of `InstanceStorage` that is `Sync` and `Send` and should make it suitable for usage with rust threads.

I hope it fits with the general plans for multithreading in #18. Feedback is more than welcome.

Co-authored-by: Jovan Gerodetti <jovan.gerodetti@titannano.de>
Co-authored-by: Jan Haller <bromeon@gmail.com>

Author: 3 people

.github/workflows/full-ci.yml

@@ -169,6 +169,12 @@ jobs:
             godot-binary: godot.linuxbsd.editor.dev.double.x86_64
             rust-extra-args: --features double-precision
 
+          - name: linux-threads
+            os: ubuntu-20.04
+            artifact-name: linux
+            godot-binary: godot.linuxbsd.editor.dev.x86_64
+            rust-extra-args: --features threads
+
           - name: linux-nightly
             os: ubuntu-20.04
             artifact-name: linux

godot-core/Cargo.toml

@@ -14,6 +14,7 @@ codegen-fmt = ["godot-ffi/codegen-fmt", "godot-codegen/codegen-fmt"]
 codegen-full = ["godot-codegen/codegen-full"]
 double-precision = ["godot-codegen/double-precision"]
 custom-godot = ["godot-ffi/custom-godot", "godot-codegen/custom-godot"]
+threads = []
 
 [dependencies]
 godot-ffi = { path = "../godot-ffi" }

godot-core/src/obj/guards.rs

@@ -4,22 +4,35 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
+#[cfg(not(feature = "threads"))]
 use std::cell;
 use std::fmt::Debug;
 use std::ops::{Deref, DerefMut};
+#[cfg(feature = "threads")]
+use std::sync;
 
 /// Immutably/shared bound reference guard for a [`Gd`][crate::obj::Gd] smart pointer.
 ///
 /// See [`Gd::bind`][crate::obj::Gd::bind] for usage.
 #[derive(Debug)]
 pub struct GdRef<'a, T> {
+    #[cfg(not(feature = "threads"))]
     cell_ref: cell::Ref<'a, T>,
+
+    #[cfg(feature = "threads")]
+    cell_ref: sync::RwLockReadGuard<'a, T>,
 }
 
 impl<'a, T> GdRef<'a, T> {
+    #[cfg(not(feature = "threads"))]
     pub(crate) fn from_cell(cell_ref: cell::Ref<'a, T>) -> Self {
         Self { cell_ref }
     }
+
+    #[cfg(feature = "threads")]
+    pub(crate) fn from_cell(cell_ref: sync::RwLockReadGuard<'a, T>) -> Self {
+        Self { cell_ref }
+    }
 }
 
 impl<T> Deref for GdRef<'_, T> {
@@ -39,13 +52,23 @@ impl<T> Deref for GdRef<'_, T> {
 /// See [`Gd::bind_mut`][crate::obj::Gd::bind_mut] for usage.
 #[derive(Debug)]
 pub struct GdMut<'a, T> {
+    #[cfg(not(feature = "threads"))]
     cell_ref: cell::RefMut<'a, T>,
+
+    #[cfg(feature = "threads")]
+    cell_ref: sync::RwLockWriteGuard<'a, T>,
 }
 
 impl<'a, T> GdMut<'a, T> {
+    #[cfg(not(feature = "threads"))]
     pub(crate) fn from_cell(cell_ref: cell::RefMut<'a, T>) -> Self {
         Self { cell_ref }
     }
+
+    #[cfg(feature = "threads")]
+    pub(crate) fn from_cell(cell_ref: sync::RwLockWriteGuard<'a, T>) -> Self {
+        Self { cell_ref }
+    }
 }
 
 impl<T> Deref for GdMut<'_, T> {

godot-core/src/storage.rs

@@ -9,16 +9,6 @@ use crate::out;
 use godot_ffi as sys;
 
 use std::any::type_name;
-use std::cell;
-
-/// Manages storage and lifecycle of user's extension class instances.
-pub struct InstanceStorage<T: GodotClass> {
-    user_instance: cell::RefCell<T>,
-
-    // Declared after `user_instance`, is dropped last
-    pub lifecycle: Lifecycle,
-    godot_ref_count: i32,
-}
 
 #[derive(Copy, Clone, Debug)]
 pub enum Lifecycle {
@@ -27,76 +17,201 @@ pub enum Lifecycle {
     Dead, // reading this would typically already be too late, only best-effort in case of UB
 }
 
-/// For all Godot extension classes
-impl<T: GodotClass> InstanceStorage<T> {
-    pub fn construct(user_instance: T) -> Self {
-        out!("    Storage::construct             <{}>", type_name::<T>());
+#[cfg(not(feature = "threads"))]
+pub use single_thread::*;
 
-        Self {
-            user_instance: cell::RefCell::new(user_instance),
-            lifecycle: Lifecycle::Alive,
-            godot_ref_count: 1,
-        }
-    }
+#[cfg(feature = "threads")]
+pub use multi_thread::*;
 
-    pub(crate) fn on_inc_ref(&mut self) {
-        self.godot_ref_count += 1;
-        out!(
-            "    Storage::on_inc_ref (rc={})     <{}>", // -- {:?}",
-            self.godot_ref_count,
-            type_name::<T>(),
-            //self.user_instance
-        );
+#[cfg(not(feature = "threads"))]
+mod single_thread {
+    use std::any::type_name;
+    use std::cell;
+
+    use crate::obj::GodotClass;
+    use crate::out;
+
+    use super::Lifecycle;
+
+    /// Manages storage and lifecycle of user's extension class instances.
+    pub struct InstanceStorage<T: GodotClass> {
+        user_instance: cell::RefCell<T>,
+
+        // Declared after `user_instance`, is dropped last
+        pub lifecycle: Lifecycle,
+        godot_ref_count: u32,
     }
 
-    pub(crate) fn on_dec_ref(&mut self) {
-        self.godot_ref_count -= 1;
-        out!(
-            "  | Storage::on_dec_ref (rc={})     <{}>", // -- {:?}",
-            self.godot_ref_count,
-            type_name::<T>(),
-            //self.user_instance
-        );
+    /// For all Godot extension classes
+    impl<T: GodotClass> InstanceStorage<T> {
+        pub fn construct(user_instance: T) -> Self {
+            out!("    Storage::construct             <{}>", type_name::<T>());
+
+            Self {
+                user_instance: cell::RefCell::new(user_instance),
+                lifecycle: Lifecycle::Alive,
+                godot_ref_count: 1,
+            }
+        }
+
+        pub(crate) fn on_inc_ref(&mut self) {
+            self.godot_ref_count += 1;
+            out!(
+                "    Storage::on_inc_ref (rc={})     <{}>", // -- {:?}",
+                self.godot_ref_count(),
+                type_name::<T>(),
+                //self.user_instance
+            );
+        }
+
+        pub(crate) fn on_dec_ref(&mut self) {
+            self.godot_ref_count -= 1;
+            out!(
+                "  | Storage::on_dec_ref (rc={})     <{}>", // -- {:?}",
+                self.godot_ref_count(),
+                type_name::<T>(),
+                //self.user_instance
+            );
+        }
+
+        /* pub fn destroy(&mut self) {
+            assert!(
+                self.user_instance.is_some(),
+                "Cannot destroy user instance which is not yet initialized"
+            );
+            assert!(
+                !self.destroyed,
+                "Cannot destroy user instance multiple times"
+            );
+            self.user_instance = None; // drops T
+                                       // TODO drop entire Storage
+        }*/
+
+        pub fn get(&self) -> cell::Ref<T> {
+            self.user_instance.try_borrow().unwrap_or_else(|_e| {
+                panic!(
+                    "Gd<T>::bind() failed, already bound; T = {}.\n  \
+                     Make sure there is no &mut T live at the time.\n  \
+                     This often occurs when calling a GDScript function/signal from Rust, which then calls again Rust code.",
+                    type_name::<T>()
+                )
+            })
+        }
+
+        pub fn get_mut(&mut self) -> cell::RefMut<T> {
+            self.user_instance.try_borrow_mut().unwrap_or_else(|_e| {
+                panic!(
+                    "Gd<T>::bind_mut() failed, already bound; T = {}.\n  \
+                     Make sure there is no &T or &mut T live at the time.\n  \
+                     This often occurs when calling a GDScript function/signal from Rust, which then calls again Rust code.",
+                    type_name::<T>()
+                )
+            })
+        }
+
+        pub(super) fn godot_ref_count(&self) -> u32 {
+            self.godot_ref_count
+        }
     }
+}
 
-    /* pub fn destroy(&mut self) {
-        assert!(
-            self.user_instance.is_some(),
-            "Cannot destroy user instance which is not yet initialized"
-        );
-        assert!(
-            !self.destroyed,
-            "Cannot destroy user instance multiple times"
-        );
-        self.user_instance = None; // drops T
-                                   // TODO drop entire Storage
-    }*/
+#[cfg(feature = "threads")]
+mod multi_thread {
+    use std::any::type_name;
+    use std::sync;
+    use std::sync::atomic::{AtomicU32, Ordering};
 
-    #[must_use]
-    pub fn into_raw(self) -> *mut Self {
-        Box::into_raw(Box::new(self))
+    use crate::obj::GodotClass;
+    use crate::out;
+
+    use super::Lifecycle;
+
+    /// Manages storage and lifecycle of user's extension class instances.
+    pub struct InstanceStorage<T: GodotClass> {
+        user_instance: sync::RwLock<T>,
+
+        // Declared after `user_instance`, is dropped last
+        pub lifecycle: Lifecycle,
+        godot_ref_count: AtomicU32,
     }
 
-    pub fn get(&self) -> cell::Ref<T> {
-        self.user_instance.try_borrow().unwrap_or_else(|_e| {
-            panic!(
-                "Gd<T>::bind() failed, already bound; T = {}.\n  \
-                 Make sure there is no &mut T live at the time.\n  \
-                 This often occurs when calling a GDScript function/signal from Rust, which then calls again Rust code.",
-                type_name::<T>()
-            )
-        })
+    /// For all Godot extension classes
+    impl<T: GodotClass> InstanceStorage<T> {
+        pub fn construct(user_instance: T) -> Self {
+            out!("    Storage::construct             <{}>", type_name::<T>());
+
+            Self {
+                user_instance: sync::RwLock::new(user_instance),
+                lifecycle: Lifecycle::Alive,
+                godot_ref_count: AtomicU32::new(1),
+            }
+        }
+
+        pub(crate) fn on_inc_ref(&mut self) {
+            self.godot_ref_count.fetch_add(1, Ordering::Relaxed);
+            out!(
+                "    Storage::on_inc_ref (rc={})     <{}>", // -- {:?}",
+                self.godot_ref_count(),
+                type_name::<T>(),
+                //self.user_instance
+            );
+        }
+
+        pub(crate) fn on_dec_ref(&mut self) {
+            self.godot_ref_count.fetch_sub(1, Ordering::Relaxed);
+            out!(
+                "  | Storage::on_dec_ref (rc={})     <{}>", // -- {:?}",
+                self.godot_ref_count(),
+                type_name::<T>(),
+                //self.user_instance
+            );
+        }
+
+        /* pub fn destroy(&mut self) {
+            assert!(
+                self.user_instance.is_some(),
+                "Cannot destroy user instance which is not yet initialized"
+            );
+            assert!(
+                !self.destroyed,
+                "Cannot destroy user instance multiple times"
+            );
+            self.user_instance = None; // drops T
+                                       // TODO drop entire Storage
+        }*/
+
+        pub fn get(&self) -> sync::RwLockReadGuard<T> {
+            self.user_instance.read().unwrap_or_else(|_e| {
+                panic!(
+                    "Gd<T>::bind() failed, already bound; T = {}.\n  \
+                     Make sure there is no &mut T live at the time.\n  \
+                     This often occurs when calling a GDScript function/signal from Rust, which then calls again Rust code.",
+                    type_name::<T>()
+                )
+            })
+        }
+
+        pub fn get_mut(&mut self) -> sync::RwLockWriteGuard<T> {
+            self.user_instance.write().unwrap_or_else(|_e| {
+                panic!(
+                    "Gd<T>::bind_mut() failed, already bound; T = {}.\n  \
+                     Make sure there is no &T or &mut T live at the time.\n  \
+                     This often occurs when calling a GDScript function/signal from Rust, which then calls again Rust code.",
+                    type_name::<T>()
+                )
+            })
+        }
+
+        pub(super) fn godot_ref_count(&self) -> u32 {
+            self.godot_ref_count.load(Ordering::Relaxed)
+        }
     }
+}
 
-    pub fn get_mut(&mut self) -> cell::RefMut<T> {
-        self.user_instance.try_borrow_mut().unwrap_or_else(|_e| {
-            panic!(
-                "Gd<T>::bind_mut() failed, already bound; T = {}.\n  \
-                 Make sure there is no &T or &mut T live at the time.\n  \
-                 This often occurs when calling a GDScript function/signal from Rust, which then calls again Rust code.",
-                type_name::<T>()
-            )
-        })
+impl<T: GodotClass> InstanceStorage<T> {
+    #[must_use]
+    pub fn into_raw(self) -> *mut Self {
+        Box::into_raw(Box::new(self))
     }
 
     pub fn mark_destroyed_by_godot(&mut self) {
@@ -127,7 +242,7 @@ impl<T: GodotClass> Drop for InstanceStorage<T> {
     fn drop(&mut self) {
         out!(
             "    Storage::drop (rc={})           <{}>", // -- {:?}",
-            self.godot_ref_count,
+            self.godot_ref_count(),
             type_name::<T>(),
             //self.user_instance
         );

godot/Cargo.toml

@@ -12,6 +12,7 @@ default = ["codegen-full"]
 formatted = ["godot-core/codegen-fmt"]
 double-precision = ["godot-core/double-precision"]
 custom-godot = ["godot-core/custom-godot"]
+threads = ["godot-core/threads"]
 
 # Private features, they are under no stability guarantee
 codegen-full = ["godot-core/codegen-full"]

itest/rust/Cargo.toml

@@ -12,6 +12,7 @@ crate-type = ["cdylib"]
 default = []
 trace = ["godot/trace"]
 double-precision = ["godot/double-precision"]
+threads = ["godot/threads"]
 
 [dependencies]
 godot = { path = "../../godot", default-features = false, features = ["formatted"] }