Skip to content

Guard _create_instance_func with _constructing_mutex on shared-global (macOS hot-reload) builds#1991

Closed
reforia wants to merge 1 commit into
godotengine:masterfrom
reforia:fix/create-instance-mutex-shared-global
Closed

Guard _create_instance_func with _constructing_mutex on shared-global (macOS hot-reload) builds#1991
reforia wants to merge 1 commit into
godotengine:masterfrom
reforia:fix/create-instance-mutex-shared-global

Conversation

@reforia
Copy link
Copy Markdown

@reforia reforia commented May 29, 2026

Bug

On macOS editor/debug builds with hot reload (MACOS_ENABLED && HOT_RELOAD_ENABLED, i.e. _GODOT_CPP_AVOID_THREAD_LOCAL), ClassDB::_create_instance_func races on the shared construction state and performs an unbalanced mutex unlock, causing intermittent (~50%) SIGABRT during off-main-thread instantiation (e.g. ResourceLoader.load_threaded_request() of a GDExtension resource, or any construction on a WorkerThreadPool thread):

BUG: Godot Object created without binding callbacks. Did you forget to use memnew()?
  (src/classes/wrapped.cpp, Wrapped::Wrapped(const StringName &))

…often followed by downstream instance-binding / TypedArray type corruption.

Full analysis in #1990.

Root cause

On this build the _constructing_* state is a shared global guarded by Wrapped::_constructing_mutex, not thread_local. The locking contract is:

  • _pre_initialize<T>() (the memnew path) locks the mutex, then sets the construct info.
  • Wrapped::Wrapped(const StringName &) consumes the globals and nulls them.
  • _postinitialize() unlocks the mutex.

Since #1568 the 4.4+ create path uses manual _set_construct_info<T>() + placement-new instead of memnew (intentionally — p_notify_postinitialize lets the engine defer post-init, which memnew can't express). But that change dropped the lock memnew was providing via _pre_initialize, while _postinitialize() still unlocks. The macOS shared-global workaround (#1594) landed between #1568's authoring and its merge, so the gap went unnoticed.

Result, under concurrency:

  1. Unguarded write_set_construct_info<T>() writes the shared globals with no lock; a concurrent construction can clobber _constructing_class_binding_callbacks before the Wrapped ctor consumes it → nullptrCRASH_NOW_MSG.
  2. Unbalanced unlock_postinitialize() unlocks a recursive_mutex this path never locked (UB), tearing down a critical section held by another thread.

_recreate_instance_func is unaffected — it already uses an explicit std::lock_guard.

Fix

Mirror _pre_initialize/_postinitialize on the 4.4+ path: lock before _set_construct_info, and balance the unlock in the deferred (no-p_notify_postinitialize) branch. The pre-4.4 path keeps memnew (which locks itself). Non-macOS and release builds compile out both #ifdef blocks — no behavior change. Nested memnew during a T constructor stays safe because the mutex is recursive.

Why the symptoms match

  • Intermittent (~50%), timing-dependent → race in a sub-microsecond unlocked window.
  • Only on threaded loads; synchronous main-thread load() never crashes → no interleaving, and a stray unlock() on an uncontended recursive_mutex is benign.
  • macOS-specific → the shared-global branch is MACOS_ENABLED && HOT_RELOAD_ENABLED only; release/export templates use real thread_local.

Testing

Builds clean against the bundled test extension on the affected config:

cd test && scons platform=macos target=editor dev_build=yes

which instantiates _create_instance_func on the _GODOT_CPP_AVOID_THREAD_LOCAL path.

Fixes #1990.

…obal builds

On macOS hot-reload builds (`_GODOT_CPP_AVOID_THREAD_LOCAL`) the `_constructing_*`
state is a shared global guarded by `Wrapped::_constructing_mutex` rather than
`thread_local`. `memnew` takes that lock via `_pre_initialize()` and releases it
in `_postinitialize()`.

Since godotengine#1568 the 4.4+ create path uses manual `_set_construct_info()` +
placement-new instead of `memnew`, so it writes the shared globals without
taking the lock while `_postinitialize()` still unlocks it: an unguarded write
plus an unbalanced unlock on the `recursive_mutex`. The macOS shared-global
workaround (godotengine#1594) landed between godotengine#1568's authoring and merge, so the create
path silently lost the lock `memnew` had been providing.

Under concurrent off-main-thread construction (e.g. threaded resource load) this
races: `_constructing_class_binding_callbacks` can be clobbered before the
`Wrapped` ctor consumes it, hitting `CRASH_NOW_MSG("...created without binding
callbacks...")`, and the stray `unlock()` tears down a critical section held by
another thread (downstream instance-binding / TypedArray corruption). It is
intermittent (~50%), macOS-only, and only on threaded loads; release/export
builds use real `thread_local` and are unaffected.

Lock before `_set_construct_info()` on the 4.4+ path and release in the
no-`p_notify_postinitialize` branch to keep the recursive mutex balanced,
mirroring `_pre_initialize()`/`_postinitialize()`. Non-macOS and release builds
compile out both blocks. `_recreate_instance_func` already uses a `lock_guard`.

Fixes godotengine#1990.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: reforia <11989463+reforia@users.noreply.github.com>
@reforia reforia requested a review from a team as a code owner May 29, 2026 17:18
@dsnopek
Copy link
Copy Markdown
Collaborator

dsnopek commented May 29, 2026

I've attempted a different approach at fixing this issue in PR #1992

It makes sure we don't have an stray mutex.lock() and mutex.unlock() that we need to keep track of, which should be easier to maintain long-term

@dsnopek
Copy link
Copy Markdown
Collaborator

dsnopek commented May 30, 2026

Superseded by #1992

@dsnopek dsnopek closed this May 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Off-main-thread instantiation crash on macOS hot-reload builds: _create_instance_func skips _constructing_mutex (unguarded write + unbalanced unlock)

2 participants