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

Improve thread safety of resource loading #88561

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Feb 19, 2024

During resource loading, including threaded loading, certain resources want to emit a changed signal. Since most of the Object API, including signals, is not thread-safe, there's the risk of this causing issues. I've actually seen it happening, but I can't be sure if not getting them anymore is due to this fix.

This PR will defer any emissions of that kind involved in loading that happen on a non-main thread. UPDATE: Bear in mind that during threaded loading, deferred calls are even more deferred, queued in a thread-specific call queue, until the resource is done loading, then flushed on the main thread.

UPDATE 2: An alternative would be letting Object itself defer signal-related calls during loads. However, I'm a bit afraid of adding a check there is too heavy for what so far seem to be few cases (more to discover, likely). Additionally, maybe at some point Object has to be made entirely thread-safe due to popular demand or something, and patches like this are not needed anymore.

@RandomShaper RandomShaper added this to the 4.3 milestone Feb 19, 2024
@RandomShaper RandomShaper requested a review from a team as a code owner February 19, 2024 17:18
@RandomShaper RandomShaper marked this pull request as draft February 23, 2024 08:59
@RandomShaper RandomShaper marked this pull request as ready for review February 23, 2024 13:26
@RandomShaper RandomShaper force-pushed the res_load_safer branch 2 times, most recently from afc6b13 to 6d30054 Compare February 26, 2024 13:41
@KoBeWi
Copy link
Member

KoBeWi commented Feb 27, 2024

Shader changes cause uniform values to not load properly at runtime:

2INkzzPfGe.mp4

BrokenShader.tscn.txt

@RandomShaper RandomShaper changed the title Improve thread-safety of resource loading Improve thread safety of resource loading Feb 28, 2024
@RandomShaper
Copy link
Member Author

@KoBeWi, thanks for reviewing. Can you confirm you have tested with the latest push I made to this branch? I'm asking because I realized issues and pushed fixes.

If you are already testing the latest, please share the MRP. I need it because with the latest changes I'm not seeing issues anymore on my side.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 28, 2024

It was with latest changes and I already shared MRP.

@RandomShaper
Copy link
Member Author

RandomShaper commented Feb 28, 2024

It was with latest changes and I already shared MRP.

I've been like reliable research, that is, doubly-bind:

  1. I couldn't spot the MRP.
  2. When I re-pushed to this branch, I did it from a source branch that hadn't been updated with the fixes.

It should be OK now.

core/io/resource.cpp Outdated Show resolved Hide resolved
core/io/resource.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some testing and it doesn't seem to break anything now, but I'm not sure how to test whether it fixes something.

@RandomShaper
Copy link
Member Author

The way I realized the issue was using multi-threaded loading, with ShaderMaterials involved. On many runs I was getting different issues, the most frequent being a failed dev assertion in Object::emit_signal() or different kinds of crashes.

@akien-mga akien-mga merged commit 7462b1a into godotengine:master Feb 29, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants