Race condition in `Resource::connect_changed`

0x0ACB commented 2 weeks ago

Tested versions

4.3

System information

Windows 11

Issue description

62d9ce6445283d2bc1daa973350f91df56a826bf 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 commented 2 weeks ago

Tested versions

4.3

Please be more explicit, you mention 4.3 but then refer to a commit which has just been merged in master for 4.4, and thus isn't in 4.3.

0x0ACB commented 2 weeks ago

Uhm the commit has been added in 4.3 stable though. And I only downloaded the 4.3-stable tag:

akien-mga commented 2 weeks ago

Ah my bad, I mistook #94526 (which includes this commit and was merged for 4.3) for #94169 (which I just merged).

RandomShaper commented 2 weeks ago

An MRP would help immensely.

0x0ACB commented 2 weeks ago

This one triggers it quite reliably for me project.zip

RandomShaper commented 2 weeks ago

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).

godotengine / godot