fix: re-acquire the GIL in class_::init_instance before instance registration

[trim21]

Description

With a factory-based py::init combined with py::call_guard<py::gil_scoped_release>, class_::init_instance runs while the GIL is released: the call_guard object is constructed in argument_loader::call_impl (cast.h) and stays alive across the construct() call that invokes init_instance. This produces two distinct failures:

GIL builds — register_instance mutates internals.registered_instances (with_instance_map has no locking on GIL builds; only free-threaded builds use a sharded mutex) concurrently with GIL-holding threads. The corruption eventually surfaces as:

terminate called after throwing an instance of 'std::runtime_error'
  what():  pybind11_object_dealloc(): Tried to deallocate unregistered instance!

Free-threaded builds — gil_scoped_release calls PyEval_SaveThread(), which detaches the thread state. init_instance then reaches PyCriticalSection_BeginMutex (via get_type_info, detail/internals.h) with no attached thread state and segfaults, deterministically, even with a single thread. The sharded instance-map mutex does not help here, because the problem is not the map race.

This affects both init_instance overloads (classic holder and smart_holder). Plain py::init<Args...> constructors are not affected, because the dispatcher calls init_instance after the call guard has already been destroyed; the same holds for the legacy def("__init__") path.

Minimal reproducer

#include <pybind11/pybind11.h>
#include <chrono>
#include <memory>
#include <thread>

namespace py = pybind11;

struct Slow {
    explicit Slow(int) {}
};

PYBIND11_MODULE(minrepro, m) {
    py::class_<Slow>(m, "Slow")
        .def(py::init([](int x) {
            // simulate a slow factory call (runs with the GIL released via the call_guard)
            std::this_thread::sleep_for(std::chrono::milliseconds(2));
            return std::make_unique<Slow>(x);
        }),
        py::arg("x"),
        py::call_guard<py::gil_scoped_release>());
}

import threading
import minrepro

def worker():
    for i in range(100):
        instance = minrepro.Slow(i)
        del instance  # deallocation runs with the GIL held

threads = [threading.Thread(target=worker) for _ in range(8)]
for t in threads:
    t.start()
for t in threads:
    t.join()

With 8 threads this aborts within a few seconds with Tried to deallocate unregistered instance!. A gdb backtrace of the abort shows pybind11_object_dealloc -> deregister_instance -> pybind11_fail while another thread is inside the GIL-released factory call, and the failing deallocation finds a corrupted/missing entry in registered_instances (the racing thread inserted entries into the unordered_multimap without the GIL, breaking the map structure). The same construction pattern segfaults on free-threaded builds even without any concurrency.

Fix

Call gil_scoped_acquire at the top of both class_::init_instance overloads. On GIL builds this is a no-op if the GIL is already held, and guarantees that register_instance and init_holder run with the GIL held. On free-threaded builds it attaches the thread state again without taking a global lock, so the instance-map critical section is entered with an attached thread state and free-threaded threads are not serialized.

The alternative of moving the call guard scope so registration happens outside it would require splitting the fused "construct + init_instance" flow of every factory py::init flavor (init.h), since the holder produced by the factory is moved into the instance by init_instance itself; acquiring inside init_instance covers all current and future call sites instead.

A regression test is added to tests/test_gil_scoped.cpp/.py: 8 threads x 100 constructions through a factory py::init with py::call_guard<py::gil_scoped_release> (with a 1 ms sleep in the factory to widen the race window). Without the fix it aborts (GIL builds) or segfaults (free-threaded builds); with the fix it passes on both, so the test is no longer skipped on free-threaded builds.

Suggested changelog entry:

• Fix class_::init_instance when a factory py::init is combined with py::call_guard<py::gil_scoped_release>: acquire the GIL (attach the thread state on free-threaded builds) before registering the instance. Previously GIL builds could abort with "Tried to deallocate unregistered instance!" and free-threaded builds could segfault, even single-threaded.

[rwgk]

Full disclosure: my mental model of the free-threaded machinery is very incomplete/vague; so I need to rely heavily on LLM help.

@colesbury for visibility

This is what codex gpt-5.6-sol medium found:

---

I found one worthwhile adjacent issue; I’d ask about it before approving.

• [P2, adjacent/pre-existing] Consider making gil_scoped_acquire unconditional in both init_instance overloads (classic holder, smart_holder). On free-threaded CPython, gil_scoped_release detaches the thread state. Compiling out the acquire leaves init_instance operating while detached. The earlier 3.14t CI run demonstrably crashed here with Fatal Python error: Segmentation fault and <freed thread state>; the current test simply skips that configuration. Although the instance map has its own mutex under Py_GIL_DISABLED, that does not restore the required attached thread state. gil_scoped_acquire should attach/detach it without introducing a global lock, so removing the #if !defined(Py_GIL_DISABLED) guards may fix both cases and allow removing the skip. I would ask the author to try that in CI.

Otherwise, the fix looks sound:

• The affected holder-producing factory path really does call init_instance while the call guard is alive.
• Both classic-holder and smart_holder overloads are covered; the smart-holder-default CI configurations exercise the latter.
• I found no other unprotected register_instance call path; all registration funnels through these overloads.
• Deregistration and instance lookup occur in paths that already hold the GIL/thread state, or have their own acquisition.
• The local master merge is clean and git diff --check passes.

Minor description nit: plain py::init<Args...> constructors do not initialize inside the guarded lambda; the dispatcher initializes them after the guard is destroyed. The problematic subset is factory forms that transfer a holder during construct(). This does not affect the code change.

[trim21]

I'm not 100% sure too, let me just test this in free threading python to see thr best option here.

[trim21]

Confirmed on my side: the acquire (thread-state attach) really is needed on free-threaded builds too, and it is not about the instance map.

gil_scoped_release calls PyEval_SaveThread(), which detaches the thread state. init_instance then reaches PyCriticalSection_BeginMutex (via get_type_info, detail/internals.h:254) with no attached thread state and segfaults — deterministically, even with a single thread. The sharded instance-map mutex does not help here because the problem is not the map race.

Verified with 3.14.7t (py::mod_gil_not_used(), sys._is_gil_enabled() == False) using the real test_gil_scoped module:

• with the #if !defined(Py_GIL_DISABLED) guards: a single-threaded SlowInit(0) segfaults; with the PY_GIL_DISABLED skip bypassed, the new test fails 5/5.
• with the guards removed (pushed as d59f5db): single-threaded OK, 8 threads x 100 constructions pass, and 8x100x1 ms finishes in ~0.11 s, so there is no global serialization. The smart_holder overload behaves the same, and it also works with PYBIND11_SIMPLE_GIL_MANAGEMENT=ON.
• GIL builds unchanged: the new test passes 10/10, and the full test_gil_scoped.py failure set is identical to before.

So I dropped the guards in both init_instance overloads. Thanks for the analysis.

[rwgk]

@trim21 could you please merge master (no specific reason; just to be up-to-date) and address the small suggestions below?

---

codex re-review:

The code concern is resolved, and the current head’s full CI is green—including Python 3.14t and 3.15t. I found no blocking correctness issue.

I would approve now. Before merging, I’d request a small prose cleanup:

• The comments in pybind11.h explain only the GIL-build race, not the newly covered free-threaded requirement to attach the thread state.
• The regression-test docstring similarly describes only the instance-map race.
• The PR description is now materially stale: it still says free-threaded builds are unchanged and the acquire is conditional. Because the suggested changelog entry becomes release documentation, that should be corrected.

[trim21]

done

[rwgk]

Thanks!