GDScript: Avoid deadlock possibility in multi-threaded load

[RandomShaper]

We already have a mechanism in WorkerThreadPool by which it collaborates with CommandQueueMT by unlocking its mutex before starting a wait and relocking it afterwards. That is a deadlock prevention mechanism that lets certain otherwise ill situations to make progress.

I found that in multi-threaded load situations (with RESOURCE_CACHE_MODE_IGNORE), the GDScript cache can incurr in a deadlock. That's the case if multiple threads interact with GDScriptCache::get_full_script(). By letting the WorkerThreadPool unlock the corresponding mutex, the deadlock is broken.

The first commit just makes the existing system more generic (mostly a refactor). The second makes GDScriptCache opt-in for it. It's interesting to keep both changes separate for bisections.

NOTE: Due to a risk of regressions, as this is slippery ground, you power maintainers may want to reschedule to 4.4.

UPDATE: Fixes #85255.

[clayjohn]

NOTE: Due to a risk of regressions, as this is slippery ground, you power maintainers may want to reschedule to 4.4.

I agree. The code seems good, but since it isn't fixing any reported regressions it's best to wait until 4.4

[RandomShaper]

This turns out to fix an important issue. We may want to reasses milestone.

[akien-mga]

This seems to break the threads=no build:

In file included from platform/web/http_client_web.cpp:31:
In file included from platform/web/http_client_web.h:34:
In file included from ./core/io/http_client.h:34:
In file included from ./core/crypto/crypto.h:36:
In file included from ./core/io/resource_loader.h:36:
./core/object/worker_thread_pool.h:240:18: error: class member cannot be redeclared
  240 |         static uint32_t thread_enter_unlock_allowance_zone(BinaryMutex *p_mutex);
      |                         ^
./core/object/worker_thread_pool.h:239:18: note: previous declaration is here
  239 |         static uint32_t thread_enter_unlock_allowance_zone(Mutex *p_mutex);
      |                         ^

[RandomShaper]

This seems to break the threads=no build:

In file included from platform/web/http_client_web.cpp:31:
In file included from platform/web/http_client_web.h:34:
In file included from ./core/io/http_client.h:34:
In file included from ./core/crypto/crypto.h:36:
In file included from ./core/io/resource_loader.h:36:
./core/object/worker_thread_pool.h:240:18: error: class member cannot be redeclared
  240 |         static uint32_t thread_enter_unlock_allowance_zone(BinaryMutex *p_mutex);
      |                         ^
./core/object/worker_thread_pool.h:239:18: note: previous declaration is here
  239 |         static uint32_t thread_enter_unlock_allowance_zone(Mutex *p_mutex);
      |                         ^

I wonder why it doesn't support the overloading. However, I can just preprocess out those for web builds. I'll push with that.

[akien-mga]

I wonder why it doesn't support the overloading. However, I can just preprocess out those for web builds. I'll push with that.

That's because in !defined(THREAD_ENABLED), those are defined to be the same:

using Mutex = MutexImpl;
using BinaryMutex = MutexImpl;

So it's the same definition twice.

[akien-mga]

Thanks!