Allowing this breaks several assumptions in the engine, normally this
doesn't occur but when using the WTP it can occur. For instance during
resource loading the message queue is serviced during tear down.
This means this situation can be created from user code if they do async
resource loading in the `_load` of a resource in the main scene.
This also makes the MainLoop report that it IS iterating during tests as
the tests expect the message queue to be serviced even though no main
loop is running.
This fixes several issues:
* There was a bug in the cycle detection. It was possible for the cycle
detection to fail because the current thread isn't in the list. Fix
this by just seeing if any of the next threads were already in the
cycle.
* By way of an optimization the thread_waiting_on bookkeeping was done
under the same thread_load_mutex as the cycle detection. But there is
also an early exit after yields. If we exit through the yield the
thread_waiting_on entry is stale and can cause other threads to fail
to do cycle detection.
* When multiple threads end up finishing a resource simultaneously
anyway, probably through cycle detection, there's a small chance that
the original thread finished more-or-less at the same time if the
original thing blocking load completed. We solve this now by letting
only one of the threads do the initial registration.
RenderingServerDefault::_draw wants to get all of the materials. The threaded
loader is still loading the materials:
1) The main thread locks canvas_singleton->shader.mutex via
RendererRD::Utilities::update_dirty_resources
-> RendererRD::MaterialStorage::_update_queued_materials
-> RendererCanvasRenderRD::CanvasMaterialData::update_parameters()
2) RendererCanvasRenderRD::CanvasMaterialData::update_parameters() wants
to get the current shader via ShaderRD::version_get_shader. This
eventually wants to wait on the load of that shader via
WorkerThreadPool::wait_for_group_task_completion in
ShaderRD::_compile_version_end
At this point the main thread is waiting for the threaded load to finish
loading the shader. Meanwhile in a thread not too far away the load is
progressing:
1) The loader is loading the tres in ResourceLoaderText::load which
eventually wants to ShaderMaterial::set_shader
2) We eventually end up in RendererCanvasRenderRD::CanvasShaderData::set_code
3) set_code wants to lock canvas_singleton->shader.mutex to protect
against concurrent compilation
At this point the main thread is waiting on the load to complete, but
holds "canvas_singleton->shader.mutex" via update_parameters(), and the
loader is waiting on "canvas_singleton->shader.mutex" in set_code.
Tragedy ensues etc.
To fix the problem we don't acquire the lock until after we're sure
we're done compiling the shader.
The same pattern exists in scene_shader_forward_clustered.cpp which
was created in the same commit as the code in the canvas singleton.
With this change the deadlock can no longer occur as set_code() can now run
concurrently with RenderingServerDefault::_draw() as the path where the
main thread waits on the workerthreadpool with
canvas_singleton->shader.mutex held can no longer occur.
Data integrity wise: previously you'd think that
canvas_singleton->shader.mutex would have protected against metadata
changes to uniforms, ubo_offsets, and texture_uniforms but set_code() used
to write to some of these (uniforms, ubo_size) without taking the lock with
no ill effects because the material isn't updated until the shader has loaded.
A similar pattern was found in scene_shader_forward_mobile.cpp and was
fixed in the same way.
Skyth
Thaddeus Crews
Hugo Locurcio
HP van Braam
Silc Lizard (Tokage) Renew
Pāvels Nadtočajevs
Edward Chan
Rémi Verschelde
kobewi
othman alzahrani
ryevdokimov
HolonProduction
Anish Kumar
Fredia Huya-Kouadio
David Snopek