Avoid TLS-destructors causing the dynamic library to not be unloaded when hot reloading #656

lilizoey · 2024-04-03T20:51:49Z

This seems to work on my end at least for linux.

By default i enable this workaround for debug builds only, as it may leak memory. However the user may override it by setting #[gdextension(enable_hot_reloading = true/false)]. This should probably be documented somewhere but im not sure the best place to do so and how to word it.

Bromeon

Thanks so much! 👍

#[gdextension(enable_hot_reloading = true/false)]

Could this be a trait function in ExtensionLibrary instead?
The logic that's in default_set_hot_reload could then become the default implementation.

The KvParser::handle_bool() could still be useful in the future, maybe you could annotate it with #[allow(dead_code)] if it's no longer used.

godot-ffi/src/linux.rs

Bromeon · 2024-04-04T07:43:09Z

godot-ffi/src/linux.rs

+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()
+    }
+}


I was thinking to check whether the extension is running in an editor or not, and only enable it if inside editor. This would likely be more robust, because hot-reloading outside editors makes no sense, however it's possible to compile Rust in release mode (for certain optimizations) and still want to hot-reload.

However, it looks like Godot APIs are not available at the time this decision needs to be made.

Any ideas on this? 🙂

maybe by default? im not sure how to do that since we need to make sure to run this code very early on in the initialization and im not sure if we can check that quite that early on

godot-ffi/src/binding/single_threaded.rs

Bromeon · 2024-04-06T13:06:52Z

godot-ffi/src/binding/single_threaded.rs

    // 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>>,


This made me think, do we still want to use the ThreadId in release mode, even if it is not used for any validation?

Pro: uses same semantics as Debug, simpler code
Con: release binaries then don't work by default with reloading (but maybe that's good to point to the override?)

Release binaries can stop working with reloading accidentally if people just add something that creates a TLS destructor that prevents the reloading anyway in this case. So i dont think that's a huge issue. Maybe we can print out a warning somehow when this potentially has happened?

A warning is a good idea 👍

It's fine if we do this at a later point too (or not at all), no need to block on this 🙂

Bromeon · 2024-04-06T13:07:13Z

godot-ffi/src/linux.rs

+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()
+    }
+}


Any ideas on this? 🙂

Bromeon · 2024-04-06T13:09:55Z

godot-ffi/src/linux.rs

+// Avoid TLS-destructors preventing the dynamic library from being closed.
+// See: https://fasterthanli.me/articles/so-you-want-to-live-reload-rust#what-can-prevent-dlclose-from-unloading-a-library


Maybe add something like "Credits to fasterthanlime for discovering the very helpful workaround." or similar, in addition to the link.

Bromeon · 2024-04-06T13:11:43Z

godot-ffi/src/linux.rs

+ * file, You can obtain one at https://mozilla.org/MPL/2.0/.
+ */
+
+//! Linux-specific configuration.


Maybe this file could be called linux_reload_workaround.rs to be more specific. If there is other Linux-specific logic in the future, we can always generalize or add a dedicated linux module.

Bromeon · 2024-04-06T13:12:53Z

godot-ffi/src/linux.rs

+        let name = c"__cxa_thread_atexit_impl".as_ptr();
+        std::mem::transmute(libc::dlsym(
+            libc::RTLD_NEXT,
+            #[allow(clippy::transmute_ptr_to_ref)]


This is needed on on the argument, too?

It seems like it's not needed at all

Bromeon · 2024-04-06T13:13:32Z

godot-ffi/src/linux.rs

+use std::ffi::c_void;
+use std::sync::OnceLock;
+
+pub type NextFn = unsafe extern "C" fn(*mut c_void, *mut c_void, *mut c_void);


Maybe ThreadAtexitFn?

Add thread-id checking back in

Bromeon

Thanks a lot!

Bromeon · 2024-04-15T10:03:14Z

godot-ffi/src/binding/single_threaded.rs

    // 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>>,


It's fine if we do this at a later point too (or not at all), no need to block on this 🙂

Bromeon · 2024-04-15T10:08:37Z

godot-core/src/init/mod.rs

+    /// 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
+    }


I might still change this later to return bool and do the release/debug check in the default impl, so that the user-facing method is simpler (there is no reason to override it and return None).

lilizoey force-pushed the fix/hot-reloading-unloading branch 3 times, most recently from b7e5936 to 1c4372a Compare April 4, 2024 00:51

Bromeon reviewed Apr 4, 2024

View reviewed changes

Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: ffi Low-level components and interaction with GDExtension API labels Apr 4, 2024

Bromeon reviewed Apr 4, 2024

View reviewed changes

godot-ffi/src/binding/single_threaded.rs Outdated Show resolved Hide resolved

lilizoey force-pushed the fix/hot-reloading-unloading branch from 1c4372a to 4f06a2a Compare April 4, 2024 13:05

Bromeon reviewed Apr 6, 2024

View reviewed changes

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

82a14f6

Add thread-id checking back in

lilizoey force-pushed the fix/hot-reloading-unloading branch from 4f06a2a to 82a14f6 Compare April 6, 2024 19:12

Bromeon approved these changes Apr 15, 2024

View reviewed changes

Bromeon added this pull request to the merge queue Apr 15, 2024

Merged via the queue into godot-rust:master with commit e7cc362 Apr 15, 2024
16 checks passed

Bromeon mentioned this pull request Jun 16, 2024

Hot reloading in Linux/macOS fails if dynamic library is not unloaded godotengine/godot#90108

Open

Bromeon mentioned this pull request Dec 18, 2024

[FIX] [WASM-Export] Unable to use multiple rust GDExtensions at the same time #973

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid TLS-destructors causing the dynamic library to not be unloaded when hot reloading #656

Avoid TLS-destructors causing the dynamic library to not be unloaded when hot reloading #656

lilizoey commented Apr 3, 2024

Bromeon left a comment

Bromeon Apr 4, 2024

Bromeon Apr 6, 2024

lilizoey Apr 6, 2024

Bromeon Apr 6, 2024

lilizoey Apr 6, 2024

Bromeon Apr 6, 2024

Bromeon Apr 15, 2024

Bromeon Apr 6, 2024

Bromeon Apr 6, 2024

Bromeon Apr 6, 2024

Bromeon Apr 6, 2024

lilizoey Apr 6, 2024

Bromeon Apr 6, 2024

Bromeon left a comment

Bromeon Apr 15, 2024

Bromeon Apr 15, 2024

		// Avoid TLS-destructors preventing the dynamic library from being closed.
		// See: https://fasterthanli.me/articles/so-you-want-to-live-reload-rust#what-can-prevent-dlclose-from-unloading-a-library

Avoid TLS-destructors causing the dynamic library to not be unloaded when hot reloading #656

Avoid TLS-destructors causing the dynamic library to not be unloaded when hot reloading #656

Conversation

lilizoey commented Apr 3, 2024

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment