godotengine / godotengine/godot

Updating `.gdextension` triggers reload, failing on macOS due to dylib staying in memory

Open
#112,202 0 comments 1 reaction 0 assignees View on GitHub
bug platform:macos topic:gdextension
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.