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

Race condition in Resource::connect_changed #96115

Closed
0x0ACB opened this issue Aug 26, 2024 · 6 comments · Fixed by #96593
Closed

Race condition in Resource::connect_changed #96115

0x0ACB opened this issue Aug 26, 2024 · 6 comments · Fixed by #96593

Comments

@0x0ACB
Copy link
Contributor

0x0ACB commented Aug 26, 2024

Tested versions

4.3

System information

Windows 11

Issue description

62d9ce6 changed Resource::connect_changed to delay signal connection if the ResourceLoader is currently loading. This leads to issues if a ResourceLoader creates a PackedScene with eg a MeshInstance3D or CollisionShape3D on a thread other than the main thread.

<core\object\object.cpp(1369): Object::connect> Condition "!p_callable.is_valid()" is true. Returning: ERR_INVALID_PARAMETER:
Cannot connect to 'changed': the provided callable is not valid: MeshInstance3D::_mesh_changed

<core\object\object.cpp(1369): Object::connect> Condition "!p_callable.is_valid()" is true. Returning: ERR_INVALID_PARAMETER:
Cannot connect to 'changed': the provided callable is not valid: Node3D::update_gizmos

Steps to reproduce

  • create a loader that creates a PackedScene with MeshInstance3D and sets a mesh
  • load a resource with the loader via `ResourceLoader::load_threaded_requet

Minimal reproduction project (MRP)

@akien-mga

This comment was marked as resolved.

@akien-mga akien-mga added this to the 4.4 milestone Aug 26, 2024
@akien-mga akien-mga moved this from Unassessed to Immediate Blocker in 4.x Release Blockers Aug 26, 2024
@0x0ACB

This comment was marked as resolved.

@akien-mga

This comment was marked as resolved.

@akien-mga akien-mga moved this from Immediate Blocker to Very Bad in 4.x Release Blockers Aug 26, 2024
@RandomShaper
Copy link
Member

An MRP would help immensely.

@0x0ACB
Copy link
Contributor Author

0x0ACB commented Aug 26, 2024

This one triggers it quite reliably for me
project.zip

@RandomShaper
Copy link
Member

RandomShaper commented Aug 30, 2024

My assessment so far:

  • This is not what we usually call a race condition, as this behavior is deterministic.
  • The only issue in a case like the one in the MRP is the warnings being printed, with no real damage. UPDATE: Not true.
  • That said, I can see why this behavior would be a problem in a more complex loader that actually needs to change resources later than being assigned (e.g., the PrimitiveMesh changed later in the MRP).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Very Bad
Development

Successfully merging a pull request may close this issue.

3 participants