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

gdext-based custom resource format tools needs experimental-threads feature #597

Open
Tracked by #713
StatisMike opened this issue Feb 6, 2024 · 2 comments
Open
Tracked by #713
Labels
bug c: engine Godot classes (nodes, resources, ...)

Comments

@StatisMike
Copy link
Contributor

StatisMike commented Feb 6, 2024

To handle custom Resource formats, one needs to define ResourceFormatSaver and ResourceFormatLoader, and register them with ResourceSaver::add_resource_format_saver() and ResourceLoader::add_resource_format_loader() for the formats to be recognizable by Godot.

Unbeknownst to the user, registered GodotClasses can be used by Godot outside of the main thread, and it happens only when opening the project in the Godot Editor (or just on InitLevel::Editor - as the same happens also during export). Currently, I can confirm only usage of ResourceFormatLoader methods, but I can't say for sure that any other calls won't be made.

Methods in question:

  • ResourceFormatLoader::get_resource_uid()
  • ResourceFormatLoader::get_recognized_extensions()
  • ResourceFormatLoader::get_resource_type()

After thread-hardening with additional checks in #581, in practice, it means that any gdext-based project that provides ResourceFormatLoader will panic without the experimental-threads feature enabled.

Will try to provide a minimal example at a further date, currently I paste below the output produced by the locally-modified gd-props integration testing project.

  • While running the empty scene without entering Godot Editor in any way:
$ godot --headless --path tests/godot
Initialize godot-rust (API v4.2.stable.official, runtime v4.2.stable.official)
Godot Engine v4.2.stable.official.46dc27791 - https://godotengine.org
 
Thread: 1 ResourceFormatLoader::get_recognized_extensions()
Thread: 1 ResourceFormatLoader::handles_type()
  • While running headless editor with --quit option (and during project export outputs seems to be analogous - EditorExportPlugin itself is running only on the main thread)
$ godot --headless --path tests/godot -e --quit
Initialize godot-rust (API v4.2.stable.official, runtime v4.2.stable.official)
Godot Engine v4.2.stable.official.46dc27791 - https://godotengine.org
 
Thread: 1 ResourceFormatLoader::get_recognized_extensions()
Thread: 1 ResourceFormatLoader::handles_type()
WARNING: Custom cursor shape not supported by this display server.
     at: cursor_set_custom_image (servers/display_server.cpp:505)
Thread: 1 ResourceFormatSaver::get_recognized_extensions()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::get_recognized_extensions()
Thread: 14 ResourceFormatLoader::get_resource_uid()
Thread: 14 ResourceFormatLoader::get_recognized_extensions()
Thread: 14 ResourceFormatLoader::get_recognized_extensions()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
  • While opening project in editor with GUI, without insta-quit
$ godot --path tests/godot -e 
Initialize godot-rust (API v4.2.stable.official, runtime v4.2.stable.official)
Godot Engine v4.2.stable.official.46dc27791 - https://godotengine.org
/lib/x86_64-linux-gnu/libxkbcommon.so.0: undefined symbol: xkb_utf32_to_keysym
/lib/x86_64-linux-gnu/libxkbcommon.so.0: undefined symbol: xkb_keymap_key_get_mods_for_level
OpenGL API 4.6 (Core Profile) Mesa 21.2.6 - Compatibility - Using Device: Intel - Mesa Intel(R) HD Graphics 520 (SKL GT2)
 
Thread: 1 ResourceFormatLoader::get_recognized_extensions()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatSaver::get_recognized_extensions()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::get_recognized_extensions()
Thread: 16 ResourceFormatLoader::get_resource_uid()
Thread: 16 ResourceFormatLoader::get_recognized_extensions()
Thread: 16 ResourceFormatLoader::get_recognized_extensions()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 1 ResourceFormatLoader::handles_type()
Thread: 17 ResourceFormatLoader::get_resource_type()
Thread: 17 ResourceFormatLoader::get_resource_type()
Thread: 17 ResourceFormatLoader::get_resource_type()
Thread: 17 ResourceFormatLoader::get_resource_type()
Thread: 17 ResourceFormatLoader::get_resource_type()
Thread: 17 ResourceFormatLoader::get_resource_type()
Thread: 17 ResourceFormatLoader::get_resource_type()
Thread: 17 ResourceFormatLoader::get_resource_type()
Thread: 17 ResourceFormatLoader::get_resource_type()
Thread: 17 ResourceFormatLoader::get_resource_type()
Thread: 17 ResourceFormatLoader::get_resource_type()
Thread: 17 ResourceFormatLoader::get_resource_type()

Browsing the project in the editor while some testing, all the methods seem to be called from the main thread.

@StatisMike
Copy link
Contributor Author

StatisMike commented Feb 8, 2024

Minimal reproductive example here.

It turned out that Editor sometimes also saves Resources using different thread.

@Bromeon
Copy link
Member

Bromeon commented Feb 12, 2024

Just my high-level perspective from Discord:

Depending on the outcome, we should maybe see if there's a way to support those without experimental-threads.
Basically, it doesn't matter when Godot runs on other threads, as long as Rust user code doesn't.

Maybe it's possible to isolate the latter somehow...

More concretely, we need to either:

  • prove that each occurrence of running Rust code from another thread is sound (because its data is isolated)
  • add the necessary safeguards to isolate such code, or defer execution to main where possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: engine Godot classes (nodes, resources, ...)
Projects
None yet
Development

No branches or pull requests

2 participants