Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hot-reload panic on Linux #653

Merged
merged 9 commits into from
Mar 30, 2024
Merged

Fix hot-reload panic on Linux #653

merged 9 commits into from
Mar 30, 2024

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Mar 30, 2024

Fixes #648.

Adds a new hot-reloading example. It's pretty bare-bones and can be polished, but that's for the future.

Also added integration tests for hot-reloading. Don't ask me how they work, it's a big patchwork of Bash, Python, GDScript, Rust and UDP 😂

Furthermore, Linux exhibits very strange reloading behavior. I cannot get normal reloading to work, since the reloaded .so library is always stale -- in both WSL2 and the CI runners. People seem to be able to do hot-reloading on Linux though. What I don't quite understand is how a .so is not fully unloaded (thus causing the double-init panic in #648), yet hot-reloading can apparently still work (in the sense that new compiled Rust code is picked up). This may need further investigation, at the moment I'm working around this issue by renaming the .so. Of course that workaround makes the scenario a bit more distant from real hot-reloading done by users.


Update: after eternal debugging, it looks like ThreadId may be responsible. It could be related to thread-local storage or syscalls that prevent dlclose() from actually closing the library. Amos' post below goes into more detail.

This also explains why the workaround experimental-threads was successful: multithreading did not validate the thread ID.

I'll leave this for later, but it's important to note that this problem is not limited to gdext. Even if we find a way to avoid ThreadId, user code could always use it. I'm not sure if other languages are also affected, but it may be necessary that Godot establishes a mechanism to rename .so files, like they already do for .dlls on Windows.

See also:

@Bromeon Bromeon added bug c: ffi Low-level components and interaction with GDExtension API c: examples Code specific to examples changed (not just follow-up from API updates) c: tooling CI, automation, tools labels Mar 30, 2024
@GodotRust
Copy link

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

@Bromeon Bromeon force-pushed the bugfix/hot-reload-panic branch 2 times, most recently from 62a15f0 to 57244e5 Compare March 30, 2024 22:55
@Bromeon
Copy link
Member Author

Bromeon commented Mar 30, 2024

Merging, as it fixes a long-standing issue with hot reload on Linux and adds integration tests, preventing further regressions.

Regarding ThreadId, a proper strategy is more sophisticated and may likely involve upstream Godot changes. As mentioned above, it might be necessary to adopt a policy to rename .so files after updating, as is already the case for Windows .dlls.

@Bromeon Bromeon enabled auto-merge March 30, 2024 23:15
@Bromeon Bromeon added this pull request to the merge queue Mar 30, 2024
Merged via the queue into master with commit 55b60fb Mar 30, 2024
16 checks passed
@Bromeon Bromeon deleted the bugfix/hot-reload-panic branch March 30, 2024 23:29
@TitanNano
Copy link
Contributor

I'm not sure if other languages are also affected, but it may be necessary that Godot establishes a mechanism to rename .so files, like they already do for .dlls on Windows.

Hot reloading on macOS does not work either. With your findings here, I get the impression the engine should enable so/dynlib/dll renaming on all platforms.

@Bromeon
Copy link
Member Author

Bromeon commented Apr 1, 2024

@TitanNano created upstream issue (see link), however simply renaming seems not viable. We probably need to address it on our side. Maybe with the macro that FasterThanLime recommends, or with some compiler flag to the LLVM backend, mirroring -fno-gnu-unique in C++.

Are you sure that macOS experiences the same issue? Not just hot-reloading doesn't work, but specifically because the dynamic library is prevented from being unloaded?

@lilizoey
Copy link
Member

lilizoey commented Apr 1, 2024

I'll see if i can hack in faster than lime's macro and maybe that just works

@lilizoey
Copy link
Member

lilizoey commented Apr 1, 2024

So it does in fact work if i add faster than lime's code (and add back in thread-id checking to actually make hot reloading fail)
Here's my code: https://github.com/lilizoey/gdextension/tree/hacky-hot-reloading-fix

@TitanNano
Copy link
Contributor

Are you sure that macOS experiences the same issue? Not just hot-reloading doesn't work, but specifically because the dynamic library is prevented from being unloaded?

@Bromeon I used 7fd1be9 on macOS and hot reloading would not work properly because the old lib version would not get unloaded. After many hours of going through fasterthanlimes post and looking into the latest available source code for dlopen and dlclose from apple, I gave up because I wasn't able to determine why the dynlib was not being unloaded.
Using the same project source on Linux showed that the project could, in fact, be hot-reloaded. It also worked on macOS when I modified the engine to use the same renaming logic as windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: examples Code specific to examples changed (not just follow-up from API updates) c: ffi Low-level components and interaction with GDExtension API c: tooling CI, automation, tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trying hot reload: Godot saying "initialize must only be called once" whenever I rebuild my Rust library
4 participants