Changes to linux_reload_workaround to allow calling is_hot_reload_enabled before enable_hot_reload by KevinThierauf · Pull Request #1040 · godot-rust/gdext

KevinThierauf · 2025-02-09T13:22:43Z

Currently linux_reload_workaround.rs stores HOT_RELOADING_ENABLED as a OnceCell, which only supports being accessed once. This means that the current implementation of is_hot_reload_enabled:

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

Sets HOT_RELOADING_ENABLED for the rest of the program, and any future calls enable_hot_reload fail. Since is_hot_reload_enabled is called by thread_atexit, thread_atexit invocations that occur before enable_hot_reload is called cause a panic.
(Unrelated but similar: disable_hot_reload notes // If hot reloading is disabled then we may call this method multiple times.; the current implementation will also cause a panic if called multiple times).

I'm specifically encountering this issue with #1037; the thread_local usage leads to is_hot_reload_enabled failing.
I used some print statements in is_hot_reload_enabled and enable_hot_reload to track down the itest failures in #1037:

> /home/user/Desktop/Godot_v4.3-stable_linux.x86_64 --path itest/godot --headless -- \[\]
is_hot_reload_enabled:
   0: godot_ffi::linux_reload_workaround::is_hot_reload_enabled
             at /home/user/Desktop/gdext-panic/godot-ffi/src/linux_reload_workaround.rs:54:44
   1: godot_ffi::linux_reload_workaround::thread_atexit
             at /home/user/Desktop/gdext-panic/godot-ffi/src/linux_reload_workaround.rs:69:8
   2: __cxa_thread_atexit_impl
             at /home/user/Desktop/gdext-panic/godot-ffi/src/linux_reload_workaround.rs:93:13
   3: std::sys::thread_local::native::eager::Storage<T>::initialize
             at /home/user/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/thread_local/native/eager.rs:47:13
   4: std::sys::thread_local::native::eager::Storage<T>::get
             at /home/user/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/thread_local/native/eager.rs:36:40
   5: godot_core::private::ERROR_CONTEXT_STACK::{{constant}}::{{closure}}
             at /home/user/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/thread_local/native/mod.rs:67:25
   6: core::ops::function::FnOnce::call_once
             at /home/user/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   7: std::thread::local::LocalKey<T>::try_with
             at /home/user/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/local.rs:282:37
   8: std::thread::local::LocalKey<T>::with
             at /home/user/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/local.rs:260:9
   9: godot_core::private::handle_panic
             at /home/user/Desktop/gdext-panic/godot-core/src/private.rs:349:5 <----- HERE
  10: godot_core::init::__gdext_load_library
             at /home/user/Desktop/gdext-panic/godot-core/src/init/mod.rs:80:22
  11: itest_init
             at /home/user/Desktop/gdext-panic/itest/rust/src/lib.rs:21:1
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: <unknown>
  16: <unknown>
  17: <unknown>
  18: <unknown>
  19: <unknown>
  20: <unknown>
  21: <unknown>
  22: __libc_start_main
  23: <unknown>

enable_hot_reload:
   0: godot_ffi::linux_reload_workaround::enable_hot_reload
             at /home/user/Desktop/gdext-panic/godot-ffi/src/linux_reload_workaround.rs:31:40
   1: godot_ffi::linux_reload_workaround::default_set_hot_reload
             at /home/user/Desktop/gdext-panic/godot-ffi/src/linux_reload_workaround.rs:47:9
   2: godot_core::init::__gdext_load_library::{{closure}}
             at /home/user/Desktop/gdext-panic/godot-core/src/init/mod.rs:32:21
   3: core::ops::function::FnOnce::call_once
             at /home/user/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   4: std::panicking::try::do_call
             at /home/user/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:557:40
   5: __rust_try
   6: std::panicking::try
             at /home/user/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:520:19
   7: std::panic::catch_unwind
             at /home/user/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:358:14
   8: godot_core::private::handle_panic
             at /home/user/Desktop/gdext-panic/godot-core/src/private.rs:355:9
   9: godot_core::init::__gdext_load_library
             at /home/user/Desktop/gdext-panic/godot-core/src/init/mod.rs:80:22
  10: itest_init
             at /home/user/Desktop/gdext-panic/itest/rust/src/lib.rs:21:1
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: <unknown>
  16: <unknown>
  17: <unknown>
  18: <unknown>
  19: <unknown>
  20: <unknown>
  21: __libc_start_main
  22: <unknown>

thread '<unnamed>' panicked at godot-ffi/src/linux_reload_workaround.rs:35:10:
hot reloading should only be set once: true

For reference, private.rs:349:5 is:

ERROR_CONTEXT_STACK.with(|cell| unsafe {   <--- HERE
        // SAFETY:
        // &error_context is valid for lifetime of function, and is removed from LAST_ERROR_CONTEXT before end of function
        cell.borrow_mut().push_function(&error_context)
    });

I don't know why, but for whatever reason the initialization of ERROR_CONTEXT_STACK, which is a thread local value, leads to thread_atexit being called. I briefly looked through the rust stdlib source; on 1.84.1 (which I used to compile), it's called by eager.rs.

In summary, it seems like using a thread local value on linux that's called before enable_hot_reload is called by __gdext_load_library leads to calling is_hot_reload_enabled, which leads to enable_hot_reload failing.

GodotRust · 2025-02-09T13:32:37Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1040

lilizoey · 2025-02-09T16:40:18Z

i dont like changing this to allow modifying this setting after it has been set. it seems much safer to me to treat all thread locals one way or another, and not potentially have some be treated one way and others another.

maybe the issue is here:

gdext/godot-core/src/init/mod.rs

Line 69 in 71bbfb8

let is_success = crate::private::handle_panic(ctx, init_code);

it might be possible for us to change this a bit so that hot reloading is set earlier than this. maybe we can change ExtensionLibrary::override_hot_reload to a const, like

pub trait ExtensionLibrary {
    const OVERRIDE_HOT_RELOAD: Option<bool> = None;
}

and then match on that value? this would guarantee that we cant panic during the execution of setting the hot reloading value, and ensure it's set before we call any of the panic handling machinery. which is where the issue seemed to come from

Bromeon · 2025-02-09T16:52:09Z

I agree with lilizoey here, as mentioned in the earlier review already:

While AtomicBool may make the issue go away, I'd rather be sure that the hot-reload-enabled flag is initialized early enough so that it's not an issue. This should be done very early, on startup. Is the whole panic handling not only set up later, or why the interference here?

it might be possible for us to change this a bit so that hot reloading is set earlier than this. maybe we can change ExtensionLibrary::override_hot_reload to a const, like

I'm wondering if anyone is using this method at all -- we could just remove it from the public API in v0.3, and use our default implemenation instead. If people complain, we can think about something.

KevinThierauf · 2025-02-10T03:08:21Z

Alternatively, could we change:

fn is_hot_reload_enabled() -> bool {
    *HOT_RELOADING_ENABLED.get_or_init(|| false)
}

into

fn is_hot_reload_enabled() -> Option<bool> {
    *HOT_RELOADING_ENABLED.get()
}

I don't think it makes sense to set HOT_RELOADING_ENABLED in a getter if the value isn't yet initialized. Option seems like a more appropriate return value.

Bromeon · 2025-02-10T13:46:13Z

I don't think it makes sense to set HOT_RELOADING_ENABLED in a getter if the value isn't yet initialized. Option seems like a more appropriate return value.

Agree with the first part, not with the second.

If anyhow possible, order of initializations should be well-defined, and cause panics whenever a violation occurs. Lazy initialization for bools should be a last resort, because it can quickly lead to non-deterministic or hard-to-debug data flows.

Concretely, is_hot_reload_enabled should probably return bool and not Option<bool>, with an expect() clause. That implies the TLS configurations are invoked after that flag has been set. But the alternative is worse: unrelated code (TLS) now has to make the call on how to interpret the tri-state true|false|None, and in the None case it might make the wrong decision because the actual information about hot-reloading comes in the future.

…KevinThierauf/gdext-panic into linux_reload_workaround-changes

KevinThierauf · 2025-02-17T08:21:08Z

Unfortunately I don't see a clear way of doing this in a way that doesn't fail cargo test. As far as I know, there's no way to detect whether tests are running at runtime, and #[cfg(test)] only applies to the crate being tested, not to the libraries.

linux_reload_workaround changes

67ebc57

KevinThierauf marked this pull request as ready for review February 9, 2025 13:50

KevinThierauf mentioned this pull request Feb 10, 2025

Panic handling: thread safety; set hook once and not repeatedly #1037

Merged

KevinThierauf and others added 10 commits February 13, 2025 23:30

revert changes, is_hot_reload_enabled panic instead of initialize

277379e

change override_hot_reload() to const

706772f

fix

eeb0c26

Merge branch 'godot-rust:master' into linux_reload_workaround-changes

56f9387

cargo fmt

8d6767f

Merge branch 'linux_reload_workaround-changes' of https://github.com/…

9e6b075

…KevinThierauf/gdext-panic into linux_reload_workaround-changes

changes

a445ee2

revert changes

c1ab323

switch init to use std::panic::catch_unwind instead of handle_panic

78833b3

added comment

ac61c57

KevinThierauf closed this Feb 17, 2025

KevinThierauf deleted the linux_reload_workaround-changes branch February 17, 2025 08:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Changes to linux_reload_workaround to allow calling is_hot_reload_enabled before enable_hot_reload#1040

Changes to linux_reload_workaround to allow calling is_hot_reload_enabled before enable_hot_reload#1040
KevinThierauf wants to merge 11 commits intogodot-rust:masterfrom
KevinThierauf:linux_reload_workaround-changes

KevinThierauf commented Feb 9, 2025 •

edited

Loading

Uh oh!

GodotRust commented Feb 9, 2025

Uh oh!

lilizoey commented Feb 9, 2025

Uh oh!

Bromeon commented Feb 9, 2025

Uh oh!

KevinThierauf commented Feb 10, 2025

Uh oh!

Bromeon commented Feb 10, 2025

Uh oh!

KevinThierauf commented Feb 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

KevinThierauf commented Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GodotRust commented Feb 9, 2025

Uh oh!

lilizoey commented Feb 9, 2025

Uh oh!

Bromeon commented Feb 9, 2025

Uh oh!

KevinThierauf commented Feb 10, 2025

Uh oh!

Bromeon commented Feb 10, 2025

Uh oh!

KevinThierauf commented Feb 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

KevinThierauf commented Feb 9, 2025 •

edited

Loading