godotengine / godotengine/godot
Updating `.gdextension` triggers reload, failing on macOS due to dylib staying in memory
- Dominant language
- C++
- Stars
- 117k
- Forks
- 26.8k
- PR merge metrics
- PR metrics pending
Description
### Tested versions
4.5-stable
### System information
macOS
### Issue description
If someone changes the `.gdextension` file, it will trigger a hot-reload of the Godot editor. On macOS, this causes a problem because the dylib has not actually changed, and unloading/reloading doesn't work as expected. (This is the TLDR, user report see below.)
We discussed this in GDExtension meeting, see https://github.com/godot-rust/gdext/pull/1367#issuecomment-3457810075, with the following consensus:
> We consider removing the changed check on the `.gdextension` file entirely:
> - Likely there are not many people depending on `.gdextension` triggering reloads.
> - The file doesn't typically change often, so when it does, an editor restart should be acceptable.
> - One use case of `.gdextension`, class-based icons, can now be supported at class registration from code directly.
> - We could add a message in the editor, when there is a change to the `.gdextension` file, telling users that an editor restart is necessary.
>
> Additionally, extension users (and binding implementors) can be encouraged to clean up global state during unloading, to reduce the impact in situation where proper unloading _doesn't_ work. At least for hot-reload workflows.
>
> A further idea from David was to detect whether unloading was successful, and display a warning/error if not.
### Steps to reproduce
From @joeyeamigh in https://github.com/godot-rust/gdext/pull/1367#issuecomment-3413257618:
> Update: I think I figured out what's going on, and why `ClassId`s persist across some hot reloads.
>
> After tracing the Godot source, it looks like there is not actually spooky action going on with `__gdext_load_library` not getting called, but rather `lldb` didn't recognize the breakpoint in the new `dylib`. Oops.
>
> What's actually happening on editor focus:
>
> * Godot schedules `GDExtensionManager::reload_extensions()`
> * `GDExtensionLibraryLoader::has_library_changed()` compares the `mtime` of both the `dylib` and the `.gdextension` file
> * (if changed) Godot calls each level's `deinitialize_library()`
> * `GDExtensionLibraryLoader::close_library()` runs, but **macOS keeps the existing image mapped if the binary is unchanged** (it appears).
>
>
> This explains why hot reload works when the code has changed (what the test checked for), but not if just the `.gdextension` file changed (what my new test checks for). Since the `dylib` stayed in memory, the second time `__gdext_load_library` is called, everything explodes. This same logic applies to clearing `MAIN_THREAD_ID`. If the shared library is being fully unloaded on other platforms then I don't think leaving the `ClassId` cache is an issue, but would love input on this since I don't know who has ownership of the memory here.
### Minimal reproduction project (MRP)
N/A
Contributor guide
Research direction
Start with GDExtensionManager::reload_extensions(), GDExtensionLibraryLoader::has_library_changed(), close_library(), and deinitialize_library(). Trace how `.gdextension` mtime changes trigger reload on macOS and review the linked discussion for the agreed direction. Done means the macOS failure is avoided, with an appropriate restart message or behavior when the `.gdextension` file changes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- game-dev, operating-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100