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.
This fixes several issues in ResourceLoader
* When waiting on resources being loaded on the main thread it is
possible that the resource was waiting on the message queue. Flush
the message queue in this situation. This was an existing bug.
* When re-starting a load for deadlock-prevention reasons, and this
type of resource has nested resources and uses resource changed
connections we end up with references to resourcese being deleted
as duplicates in the dependent task. We now keep a reference to these
resources for the duration of the dependent load task. This closes
the race but does not entirely eliminate it during intial import.
This is an existing bug seemingly specific to TileSetSource but there
might be others.
* When threads are waiting during teardown we ended up deadlocking
because the waiting thread would never end up completing. This is a
bug introduced by f63ab5f
* When a thread loading a resource ends up calling back into the
WorkerThreadPool to wait cooperatively it expects that other threads
complete, or new requests are made, so that work moves forward. If
all OTHER threads end up blocked by the WorkerThreadPool waiting
thread then no forward progress can be made anymore. We now solve
this problem by yielding whenever we are blocked by another task. We
wake up each every task whenever any progress is made to see if
further progress can be made. This is a bug introduced by f63ab5f
* When dependencies get introduced while loading it is possible to form
a dependency cycle. Normally this isn't a problem but when all
threads are already waiting when the cycle is introduced no forward
progress can be made anymore. We now break such cycles. This is a bug
introduced by f63ab5f
When multiple threads try to load the same sub-resource the
ResourceLoader's anti-deadlock protection thinks that the other threads
loading the dependency are old threads blocked on a dependency.
The current thread will then immediately start to load the sub-resource
since they can't wait for it. This can then lead to multiple threads
loading the same resource concurrently.
We fix the problem by recording in the task when loading has ACTUALLY
started, and if so we simply wait.
This is no worse than before, at the point where we are waiting now we
previously re-started the load entirely.
Tested by running the MRP from the below issue (with and without asan
and msan), and loading and running various public and private Godot
projects.
This also fixes a use-after-free on resource_changed_connections when
multiple threads end up running the same import. This is a pre-existing
bug but this code widened the race window.
This also fixes a pre-existing memory leak in load_threaded_request,
when the user calls the function we capture the Ref<LoadToken> but
immediately drop the ref. Causing the refcount to go to 1. Later in
_run_load_task we unreference manually, bringing the refcount to 0. This
then later prevents the Ref<> from memdeleting the LoadToken.
This fixes#118085
Ensures all resource types support UIDs in a project.
This is required to fix:
* Scripts and many other resource types can't be referenced by UID and when refactored the references are lost.
* Path export properties can't use UID for unsupported types.
* Refactoring problems when files are moved outside the editor (this PR effectively fixes it).
* Editor properly refreshing paths if they changed externally while opened (as example, git update).
This needs to be addressed in a subsequent PR, but this one effectively sets the prerequisites.
Resource types that do not support UID will get a .uid file appended to them (this includes .gd, .gdshader, .gdextension, etc. files).
When exporting a project, resources are often moved, converted or
remapped (source import files are gone, text scenes are converted to binary,
etc).
This new function allows to list a directory and obtain the actual original
resource files.
Example
```GDScript
var resources = ResourceLoader.list_directory("res://images")
print(resources)
```
Result will be something like:
```
["image1.png","image2.png","image3.png"]
```
instead of
```
["image1.png.import","image2.png.import","image3.png.import"]
```
1. Make handling of user tokens atomic:
Loads started with the external-facing API used to perform a two-step setup of the user token. Between both, the mutex was unlocked without its reference count having been increased. A non-user-initiated load could therefore destroy the load task when it unreferenced the token.
Those stages now happen atomically so in the one hand, the described race condition can't happen so the load task life insurance doesn't have a gap anymore and, on the other hand, the ugliness that the call to load could return `ERR_BUSY` if happening while other thread was between both steps is gone.
The code has been refactored so the user token concerns are still outside the inner load start function, which is agnostic to that for a cleaner implementation.
2. Clear ambiguity between load operations running on `WorkerThreadPool`:
The two cases are: single-loaded thread directly started by a user pool task and a load started by the system as part of a multi-threaded load.
Since ensuring all the code dealing with this distinction would make it very complex, and error-prone, a different measure is applied instead: just take one of the cases out of the dicotomy. We now ensure every load happening on a pool thread has been initiated by the system.
The way of achieving that is that a single-threaded user-started load initiated from a pool thread, is run as another task.
Benefits:
- Simpler code. The main load function is renamed so it's apparent that it's not just a thread entry point anymore.
- Cache and thread modes of the original task are honored. A beautiful consequence of this is that, unlike formerly, re-issued loads can use the resource cache, which makes this mechanism much more performant.
- The newly added getter for caller task id in WorkerThreadPool allows to remove the custom tracking of that in ResourceLoader.
- The check to replace a cached resource and the replacement itself happen atomically. That fixes deadlock prevention leading to multiple resource instances of the same one on disk. As a side effect, it also makes the regular check for replace load mode more robust.
ResourceLoader:
- Fix invalid tokens being returned.
- Remove no longer written `ThreadLoadTask::dependent_path` and the code reading from it.
- Clear deadlock hazard by keeping the mutex unlocked during userland polling.
WorkerThreadPool:
- Include thread call queue override in the thread state reset set, which allows to simplify the code that handled that (imperfectly) in the ResourceLoader.
- Handle the mutex type correctly on entering an allowance zone.
CommandQueueMT:
- Handle the additional possibility of command buffer reallocation that mutex unlock allowance introduces.
- Unify documentation, hoping to clear misconcepctions about about propagation of the cache mode across dependant loads.
- Clarify in docs that `CACHE_MODE_REPLACE` now also works on the main resource (from #87008).
- Add two recursive modes, counterparts of `CACHE_MODE_REPLACE` and `CACHE_MODE_IGNORE`, since it seems some need them (see #59669, #82830).
- Let resources, even loaded with one of the ignore-cache modes, get a path, which is useful for tools.
This change simply extracts 'SafeBinaryMutex' from 'mutex.h' to
'safe_binary_mutex.h', in an effort to reduce the compilation
speed impact of including `mutex.h`.
This allows to include script_instance.h directly in the
generated gdvirtual.gen.inc, and remove excessive includes
from the codebase.
This should also allow Resource to use GDVIRTUAL macros,
which wasn't possible previously due to a circular dependency.
HP van Braam
StarryWorm
Jules
Lukas Tenbrink
Thaddeus Crews
Pedro J. Estébanez
Haoyu Qiu
Jojo-1000
RedMser
Pāvels Nadtočajevs
kobewi
Tareq Anuar
Yufeng Ying
Juan
Adam Scott
Rémi Verschelde
Aleksey Vasenev
A Thousand Ships
Muller-Castro
vittorioromeo
Saracen
Yuri Sizov
Raul Santos