This turned out to break a lot of things in the engine unexpectedly. The
actual deadlock that this fixes seems rare so a better approach is
needed.
For 4.7 we should drop this one.
This reverts commit ae564feb2a.
This reverts commit 7ab8328204.
When the main thread is waiting on a worker thread pool via
WorkerThreadPool::wait_for_group_task_completion() the MessageQueue is no
longer being serviced.
But in ResourceLoader::_load_complete_inner we need to be able to push a
callable onto the MessageQueue and we wait until completion. Effectively
waiting on the main thread to run our code.
If the tasking waiting on completion is in the group the main thread is
waiting for then this can never happen.
We solve this problem by servicing the MessageQueue in the
WorkerThreadPool if main thread is waiting. To avoid busy waiting on the
semaphore we sleep for a very brief time to spare battery without impacting
latency too much in the waiting case.
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.
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
The `Engine::Singleton` struct stores a raw `Object *`, so registering
a `RefCounted` only works if some external `Ref<>` keeps it alive. This
pattern is fragile and can lead to UB, especially with GDExtension.
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
These conflict with `ui_text_completion_replace` (accept autocompletion), but
the latter seems to take precedence if an autocomplete popup is open, so this
still works as expected.
Fixes#119085
This method is used to generate headers for embedding files into the binary
(think about the new `#embed` feature in C23 and C++26).
While the stringification step itself was plenty fast, it then proceeded
to wrap everything using the `textwrap` module. `textwrap` is *very*
slow, as it's apparently optimized for human text.
This patch reimplements the wrapping logic using a simple regex,
resulting in a tremendous speed improvement (~6x), and switches to `map`
for the stringification itself (thanks Rémi!)
It also removes a (practically) unused argument, `initial_indent`.
The generated files are pretty much the same, with a tiny difference in
line length (for some reason the old logic overshot the requested line
length)
Thaddeus Crews
Pāvels Nadtočajevs
Lukas Tenbrink
HP van Braam
Edward Chan
Cyklon_3000
nikitalita
Ivan Šachov
Fredia Huya-Kouadio
Jan Haller
Manpreet Singh
Dimitri Sukhankin
Enzo Novoselic
Mikael Hermansson
Thomas ten Cate
HolonProduction
Dery Almas
bucket
Mister Ky
David Snopek