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

[reforia]

Summary

On macOS editor/debug builds with hot reload enabled (MACOS_ENABLED && HOT_RELOAD_ENABLED), 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 of GDExtension objects (e.g. ResourceLoader.load_threaded_request() of an extension resource, or any construction on a WorkerThreadPool thread).

The crash signature is:

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 container/type corruption (e.g. a wrong-typed pointer landing in a TypedArray<T>).

Root cause

On this build configuration the construction state is shared across threads, not thread_local, and is guarded by Wrapped::_constructing_mutex:

// include/godot_cpp/classes/wrapped.hpp
#if defined(MACOS_ENABLED) && defined(HOT_RELOAD_ENABLED)
#define _GODOT_CPP_AVOID_THREAD_LOCAL
#define _GODOT_CPP_THREAD_LOCAL          // -> not thread_local
#else
#define _GODOT_CPP_THREAD_LOCAL thread_local
#endif

_GODOT_CPP_THREAD_LOCAL static const StringName *_constructing_extension_class_name;
_GODOT_CPP_THREAD_LOCAL static const GDExtensionInstanceBindingCallbacks *_constructing_class_binding_callbacks;

The locking contract is:

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

So the critical section is meant to span the entire construction, and memnew (_pre_initialize + _post_initialize) honors it. _create_instance_func does not:

// include/godot_cpp/core/class_db.hpp  (Godot 4.4+ path)
template <typename T>
static GDExtensionObjectPtr _create_instance_func(void *data, GDExtensionBool p_notify_postinitialize) {
    if constexpr (!std::is_abstract_v<T>) {
        Wrapped::_set_construct_info<T>();   // (a) writes the SHARED globals with NO lock
        T *new_object = new ("", "") T;      //     Wrapped ctor consumes them
        if (p_notify_postinitialize) {
            new_object->_postinitialize();   // (b) UNLOCKS a mutex this path never locked
        }
        return new_object->_owner;
    } else {
        return nullptr;
    }
}

Two defects under concurrency on the _GODOT_CPP_AVOID_THREAD_LOCAL build:

1. Unguarded write. It calls _set_construct_info<T>() directly instead of _pre_initialize<T>(), so it never takes _constructing_mutex. A concurrent construction on another thread can clobber _constructing_class_binding_callbacks between this thread setting it and the Wrapped ctor consuming it → ctor sees nullptr → CRASH_NOW_MSG.
2. Unbalanced unlock. When p_notify_postinitialize is true, _postinitialize() calls _constructing_mutex.unlock() on a recursive_mutex this path never locked — UB, and it tears down a critical section legitimately held by another thread, letting two threads mutate the shared globals and the instance-binding table simultaneously (the source of the downstream TypedArray corruption).

_recreate_instance_func is not affected — it uses an explicit std::lock_guard<std::recursive_mutex>, so it's balanced. Only the create path is broken.

How it got here (for context)

_create_instance_func was changed from memnew(T) to manual _set_construct_info + placement-new in #1568, intentionally — create_instance2 / GDExtensionClassCreationInfo4 introduced p_notify_postinitialize to let the engine defer the POSTINITIALIZE notification, which memnew (always post-initializes) can't express. The macOS shared-global mutex workaround (#1594) landed between #1568's authoring and its merge, so the create path lost the lock that memnew's _pre_initialize had been providing, while keeping the unlock in _postinitialize. There's no record of the mutex being considered in #1568 — it appears to be a merge-ordering oversight rather than a deliberate change.

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 → single-threaded means no interleaving, and a stray unlock() on an otherwise-uncontended recursive_mutex is benign. ✔
• macOS-specific → the shared-global branch is MACOS_ENABLED && HOT_RELOAD_ENABLED only; release/export-template builds use real thread_local (the #else branch) and do not crash. ✔

Proposed fix

Mirror _pre_initialize/_postinitialize in the create path: lock before _set_construct_info, and balance the unlock in the deferred (no-postinit) branch.

template <typename T>
static GDExtensionObjectPtr _create_instance_func(void *data, GDExtensionBool p_notify_postinitialize) {
    if constexpr (!std::is_abstract_v<T>) {
#ifdef _GODOT_CPP_AVOID_THREAD_LOCAL
        Wrapped::_constructing_mutex.lock();
#endif
        Wrapped::_set_construct_info<T>();
        T *new_object = new ("", "") T;
        if (p_notify_postinitialize) {
            new_object->_postinitialize();   // releases the lock
        }
#ifdef _GODOT_CPP_AVOID_THREAD_LOCAL
        else {
            Wrapped::_constructing_mutex.unlock();   // balance when post-init is deferred
        }
#endif
        return new_object->_owner;
    } else {
        return nullptr;
    }
}

This serializes the create path on the same mutex as memnew on the affected build, closing the unguarded-write window and the unbalanced unlock. Non-macOS / release builds are unaffected (both #ifdef blocks compile out). Nested memnew during a T constructor remains safe because _constructing_mutex is recursive. Builds clean against master's test extension on platform=macos target=editor dev_build=yes (the _GODOT_CPP_AVOID_THREAD_LOCAL config).

Happy to open a PR if this looks right.

System / build

• Platform: macOS
• Build: target=editor/template_debug with hot reload (HOT_RELOAD_ENABLED) — i.e. the _GODOT_CPP_AVOID_THREAD_LOCAL path
• godot-cpp: current master (verified the create path still acquires no mutex)

Steps to reproduce

1. Register a custom GDExtension Resource subclass and create several .tres instances of it.
2. On a macOS editor/debug build, issue ResourceLoader.load_threaded_request() for all of them concurrently (so deserialization/construction runs on WorkerThreadPool threads).
3. Boot headless repeatedly. Observe intermittent SIGABRT with the without binding callbacks signature; building a release/export template makes it go away.

[dsnopek]

Thanks!

This issue appears to be AI generated, and is quite difficult to follow due to its verbosity, but I think I can confirm this is a valid issue.

However, I'm not crazy about the proposed solution - it's making a messy situation around locking even messier.

I've attempted a different solution in PR #1992

If you have the time to test it, I'd appreciate it :-)

[reforia]

Thanks!

This issue appears to be AI generated, and is quite difficult to follow due to its verbosity, but I think I can confirm this is a valid issue.

However, I'm not crazy about the proposed solution - it's making a messy situation around locking even messier.

I've attempted a different solution in PR #1992

If you have the time to test it, I'd appreciate it :-)

Absolutely, I will test the solution and report back.

P.s Sorry for the verbosity, the ticket was indeed AI generated but the issue and debugging process was me scratching my head on my other projects and investigated together with AI, but next time I will make it less verbose and easier to follow

[reforia]

So I tested #1992's per-thread ConstructInfo approach on macOS (editor/dev, hot reload). The fix looks good in my cases — sharing two quick checks in case they're useful.

1. Race-free under contention. Extracted the ConstructInfoStore pattern into a standalone harness and hammered it with ThreadSanitizer: 16 threads × 50k constructions each (800k total), 0 data races, 0 cross-thread clobbering. A shared-global control (the old behavior, no per-thread isolation) fires TSan immediately, so the pass is real. Returning the map reference and using it unlocked is safe — each thread only ever touches its own node's value.

2. The dylib actually unloads. Since the whole reason thread_local was avoided on macOS is that it pins the image and breaks unload, I checked that directly with dlopen → use from a worker thread → dlclose → dlopen(RTLD_NOLOAD):

build	after dlclose
thread_local slot	still resident (unload blocked)
#1992 map slot	unloaded cleanly

So #1992 keeps the unload property while dropping the construction-spanning lock. Builds clean here, lib + test extension.

With that, I haven't done a full in-editor reload cycle (class re-register + live-instance migration) yet — but the dylib-unload blocker, which was the original motivation is gone. So I will keep #1992 locally for a while and monitor it further, and report back if anything regressed

Thanks!