-
-
Notifications
You must be signed in to change notification settings - Fork 251
Fix Hot Reload on MacOS #1367
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
base: master
Are you sure you want to change the base?
Fix Hot Reload on MacOS #1367
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your contribution! 👍
I wrote down a question about reload semantics, maybe it's also a misunderstanding on my side.
fn clear(&mut self) { | ||
self.entries.clear(); | ||
// Don't clear entries - ClassId objects with old indices may still exist and need to access them. | ||
// This is especially important during hot reload where static ClassIds persist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you happen to find out why static ClassId
s persist across hot reloads? This indicates that the library is not properly unloaded before being reloaded.
I fear while this may fix this specific problem, there are many instances of other statics: in godot-rust, in other Rust libraries, and in user code. I don't see it realistic to expect from the user to not use static
(or at least not without custom cleanup logic), if this works well on other platforms.
So I'm wondering if this is is not just fighting symptoms, and it's somehow possible to fix the root cause 🤔 what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MacOS doesn't allow for unloading dylibs, which makes this a difficult problem to solve.
I definitely agree that the dylib is not unloading properly, but from my experimentation this seemed to apply to editor-level code. If I initialized tracing_subscriber in the impl ExtensionLibrary, there would already be a global subscriber on next reload. When I moved the subscriber to only init on the Scene level though, I stopped having collisions. I didn't spend that much time looking into it though, and made other changes around that time so it's possible I elided the [tracing-subscriber] bug another way.
Simply clearing the Thread ID allows for hot reloading (as I was hitting a set() panic), and I stopped clearing the ClassIds because the editor would note non-fatal reference errors after the reload. It doesn't seem to cause any Id collision however, so I chalked it up to an acceptable slow memory leak in development.
I can attach a debugger and dig further into this in the next few days for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoeyEamigh can you replicate your issue with our hot-reloading demo project? Because I can't. Hot reloading works fine with our example even on macOS, so I suspect that there is some specific code that breaks hot reloading.
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1367 |
@TitanNano (bringing this out of the code review comments), you were correct that the hot-reload demo does not trigger the crash. I dug into it and was able to create a very simple additional test case for the hot-reload demo that causes the crash consistently for me. master...JoeyEamigh:gdext:test_hot_reload_gdextension_edited. Basically, I discovered that the crash occurs when the I am currently unsure if this is related to the non-fatal godot Not quite sure what the right thing to do here is, but hopefully with the test case you will be able to see where I'm coming from? Bromeon, you are definitely correct that my initial PR's removal of clearing ClassIds is just fighting the symptoms. The Logs on `cargo build -p hot-reload --features godot/api-4-5,godot/__debug-log` reload
Call stack to `GDExtensionManager::reload_extensions` on `cargo build -p hot-reload --features godot/api-4-5,godot/__debug-log` reload
Logs on `touch rust.gdextension` reload
Call stack to `__gdext_load_library` on `touch rust.gdextension` reload (lightly cleaned)
(EDIT by Yarwin – formatting in last |
I have been testing this patch for the past two days on both MacOS Sonoma and MacOS Tahoe, and it seems to fix the issues that I was seeing with hot reload in #1364. I unfortunately do not have a Windows machine to test on, so I am not sure about knock-on effects of this code change. If this change adversely affects Windows, it would likely be prudent to split the MacOS hot reload out similar to the Linux hot reload.
Thank you for this amazing project! Let me know if there is anything I can do, or if I need to split out the hot reload functionality for MacOS.