Add faster-than-lime's workaround for hot reloading unloading library

Add thread-id checking back in

Author: lilizoey

godot-core/src/init/mod.rs

@@ -24,6 +24,16 @@ pub unsafe fn __gdext_load_library<E: ExtensionLibrary>(
     init: *mut sys::GDExtensionInitialization,
 ) -> sys::GDExtensionBool {
     let init_code = || {
+        // Make sure the first thing we do is check whether hot reloading should be enabled or not. This is to ensure that if we do anything to
+        // cause TLS-destructors to run then we have a setting already for how to deal with them. Otherwise this could cause the default
+        // behavior to kick in and disable hot reloading.
+        #[cfg(target_os = "linux")]
+        match E::override_hot_reload() {
+            None => sys::linux_reload_workaround::default_set_hot_reload(),
+            Some(true) => sys::linux_reload_workaround::enable_hot_reload(),
+            Some(false) => sys::linux_reload_workaround::disable_hot_reload(),
+        }
+
         let tool_only_in_editor = match E::editor_run_behavior() {
             EditorRunBehavior::ToolClassesOnly => true,
             EditorRunBehavior::AllClasses => false,
@@ -209,6 +219,20 @@ pub unsafe trait ExtensionLibrary {
     fn on_level_deinit(level: InitLevel) {
         // Nothing by default.
     }
+
+    /// Whether to enable hot reloading of this library. Return `None` to use the default behavior.
+    ///
+    /// Enabling this will ensure that the library can be hot reloaded. If this is disabled then hot reloading may still work, but there is no
+    /// guarantee. Enabling this may also lead to memory leaks, so it should not be enabled for builds that are intended to be final builds.
+    ///
+    /// By default this is enabled for debug builds and disabled for release builds.
+    ///
+    /// Note that this is only checked *once* upon initializing the library. Changing this from `true` to `false` will be picked up as the
+    /// library is then fully reloaded upon hot-reloading, however changing it from `false` to `true` is almost certainly not going to work
+    /// unless hot-reloading is already working regardless of this setting.
+    fn override_hot_reload() -> Option<bool> {
+        None
+    }
 }
 
 /// Determines if and how an extension's code is run in the editor.

godot-ffi/Cargo.toml

@@ -21,6 +21,9 @@ trace = []
 [dependencies]
 paste = "1"
 
+[target.'cfg(target_os = "linux")'.dependencies]
+libc = "0.2.153"
+
 [target.'cfg(target_family = "wasm")'.dependencies]
 gensym = "0.1.1"
 

godot-ffi/src/binding/single_threaded.rs

@@ -10,13 +10,14 @@
 //! If used from different threads then there will be runtime errors in debug mode and UB in release mode.
 
 use std::cell::Cell;
+use std::thread::ThreadId;
 
 use super::GodotBinding;
 use crate::ManualInitCell;
 
 pub(super) struct BindingStorage {
     // Is used in to check that we've been called from the right thread, so must be thread-safe to access.
-    is_initialized: Cell<bool>,
+    main_thread_id: Cell<Option<ThreadId>>,
     binding: ManualInitCell<GodotBinding>,
 }
 
@@ -29,7 +30,7 @@ impl BindingStorage {
     #[inline(always)]
     unsafe fn storage() -> &'static Self {
         static BINDING: BindingStorage = BindingStorage {
-            is_initialized: Cell::new(false),
+            main_thread_id: Cell::new(None),
             binding: ManualInitCell::new(),
         };
 
@@ -48,11 +49,9 @@ impl BindingStorage {
         // in which case we can tell that the storage has been initialized, and we don't access `binding`.
         let storage = unsafe { Self::storage() };
 
-        let was_initialized = storage.is_initialized.replace(true);
-        assert!(
-            !was_initialized,
-            "initialize must only be called at startup or after deinitialize"
-        );
+        storage
+            .main_thread_id
+            .set(Some(std::thread::current().id()));
 
         // SAFETY: We are the first thread to set this binding (possibly after deinitialize), as otherwise the above set() would fail and
         // return early. We also know initialize() is not called concurrently with anything else that can call another method on the binding,
@@ -71,8 +70,12 @@ impl BindingStorage {
         // SAFETY: We only call this once no other operations happen anymore, i.e. no other access to the binding.
         let storage = unsafe { Self::storage() };
 
-        let was_initialized = storage.is_initialized.replace(false);
-        assert!(was_initialized, "deinitialize without prior initialize");
+        storage
+            .main_thread_id
+            .get()
+            .expect("deinitialize without prior initialize");
+
+        storage.main_thread_id.set(None);
 
         // SAFETY: We are the only thread that can access the binding, and we know that it's initialized.
         unsafe {
@@ -89,8 +92,7 @@ impl BindingStorage {
     pub unsafe fn get_binding_unchecked() -> &'static GodotBinding {
         let storage = Self::storage();
 
-        // FIXME currently disabled as it breaks hot-reloading on Linux, see https://github.com/godot-rust/gdext/pull/653.
-        /*if cfg!(debug_assertions) {
+        if cfg!(debug_assertions) {
             let main_thread_id = storage.main_thread_id.get().expect(
                 "Godot engine not available; make sure you are not calling it from unit/doc tests",
             );
@@ -100,7 +102,7 @@ impl BindingStorage {
                 std::thread::current().id(),
                 "attempted to access binding from different thread than main thread; this is UB - use the \"experimental-threads\" feature."
             );
-        }*/
+        }
 
         // SAFETY: This function can only be called when the binding is initialized and from the main thread, so we know that it's initialized.
         unsafe { storage.binding.get_unchecked() }
@@ -109,7 +111,7 @@ impl BindingStorage {
     pub fn is_initialized() -> bool {
         // SAFETY: We don't access the binding.
         let storage = unsafe { Self::storage() };
-        storage.is_initialized.get()
+        storage.main_thread_id.get().is_some()
     }
 }
 

godot-ffi/src/lib.rs

@@ -35,6 +35,8 @@ mod compat;
 mod extras;
 mod global;
 mod godot_ffi;
+#[cfg(target_os = "linux")]
+pub mod linux_reload_workaround;
 mod opaque;
 mod plugins;
 mod string_cache;

godot-ffi/src/linux_reload_workaround.rs

@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) godot-rust; Bromeon and contributors.
+ * 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/.
+ */
+
+//! Linux-specific configuration.
+
+// Avoid TLS-destructors preventing the dynamic library from being closed.
+//
+// Credits to fasterthanlime for discovering the very helpful workaround.
+// See: https://fasterthanli.me/articles/so-you-want-to-live-reload-rust#what-can-prevent-dlclose-from-unloading-a-library
+
+use std::ffi::c_void;
+use std::sync::OnceLock;
+
+pub type ThreadAtexitFn = unsafe extern "C" fn(*mut c_void, *mut c_void, *mut c_void);
+
+static SYSTEM_THREAD_ATEXIT: OnceLock<Option<ThreadAtexitFn>> = OnceLock::new();
+static HOT_RELOADING_ENABLED: OnceLock<bool> = OnceLock::new();
+
+fn system_thread_atexit() -> &'static Option<ThreadAtexitFn> {
+    SYSTEM_THREAD_ATEXIT.get_or_init(|| unsafe {
+        let name = c"__cxa_thread_atexit_impl".as_ptr();
+        std::mem::transmute(libc::dlsym(libc::RTLD_NEXT, name))
+    })
+}
+
+pub fn enable_hot_reload() {
+    // If hot reloading is enabled then we should properly unload the library, so this will only be called once.
+    HOT_RELOADING_ENABLED
+        .set(true)
+        .expect("hot reloading should only be set once")
+}
+
+pub fn disable_hot_reload() {
+    // If hot reloading is disabled then we may call this method multiple times.
+    _ = HOT_RELOADING_ENABLED.set(false)
+}
+
+pub fn default_set_hot_reload() {
+    // By default we enable hot reloading for debug builds, as it's likely that the user may want hot reloading in debug builds.
+    // Release builds however should avoid leaking memory, so we disable hot reloading support by default.
+    if cfg!(debug_assertions) {
+        enable_hot_reload()
+    } else {
+        disable_hot_reload()
+    }
+}
+
+fn is_hot_reload_enabled() -> bool {
+    // Assume hot reloading is disabled unless something else has been specified already. This is the better default as thread local storage
+    // destructors exist for good reasons.
+    // This is needed for situations like unit-tests, where we may create TLS-destructors without explicitly calling any of the methods
+    // that set hot reloading to be enabled or disabled.
+    *HOT_RELOADING_ENABLED.get_or_init(|| false)
+}
+
+/// Turns glibc's TLS destructor register function, `__cxa_thread_atexit_impl`,
+/// into a no-op if hot reloading is enabled.
+///
+/// # Safety
+/// This needs to be public for symbol visibility reasons, but you should
+/// never need to call this yourself
+pub unsafe fn thread_atexit(func: *mut c_void, obj: *mut c_void, dso_symbol: *mut c_void) {
+    if is_hot_reload_enabled() {
+        // Avoid registering TLS destructors on purpose, to avoid
+        // double-frees and general crashiness
+    } else if let Some(system_thread_atexit) = *system_thread_atexit() {
+        // Hot reloading is disabled, and system provides `__cxa_thread_atexit_impl`,
+        // so forward the call to it.
+        system_thread_atexit(func, obj, dso_symbol);
+    } else {
+        // Hot reloading is disabled *and* we don't have `__cxa_thread_atexit_impl`,
+        // throw hands up in the air and leak memory.
+    }
+}
+
+#[macro_export]
+macro_rules! register_hot_reload_workaround {
+    () => {
+        #[no_mangle]
+        pub unsafe extern "C" fn __cxa_thread_atexit_impl(
+            func: *mut ::std::ffi::c_void,
+            obj: *mut ::std::ffi::c_void,
+            dso_symbol: *mut ::std::ffi::c_void,
+        ) {
+            $crate::linux_reload_workaround::thread_atexit(func, obj, dso_symbol);
+        }
+    };
+}

godot-macros/src/gdextension.rs

@@ -100,5 +100,8 @@ pub fn attribute_gdextension(item: venial::Item) -> ParseResult<TokenStream> {
             // Ensures that the init function matches the signature advertised in FFI header
             let _unused: ::godot::sys::GDExtensionInitializationFunction = Some(#entry_point);
         }
+
+        #[cfg(target_os = "linux")]
+        ::godot::sys::register_hot_reload_workaround!();
     })
 }

godot-macros/src/util/kv_parser.rs

@@ -161,6 +161,31 @@ impl KvParser {
         Ok(Some(int))
     }
 
+    #[allow(dead_code)]
+    pub fn handle_bool(&mut self, key: &str) -> ParseResult<Option<bool>> {
+        let Some(expr) = self.handle_expr(key)? else {
+            return Ok(None);
+        };
+
+        let mut tokens = expr.into_iter();
+        let Some(TokenTree::Ident(id)) = tokens.next() else {
+            return bail!(key, "missing value for '{key}' (must be bool literal)");
+        };
+
+        if let Some(surplus) = tokens.next() {
+            return bail!(
+                key,
+                "value for '{key}' must be bool literal; found extra {surplus:?}"
+            );
+        }
+
+        let Ok(b) = id.to_string().parse() else {
+            return bail!(key, "value for '{key}' must be bool literal; found {id:?}");
+        };
+
+        Ok(Some(b))
+    }
+
     /// Handles a key that must be provided and must have an identifier as the value.
     pub fn handle_ident_required(&mut self, key: &str) -> ParseResult<Ident> {
         self.handle_ident(key)?