Fix unit tests

Author: b-pass

include/pybind11/detail/internals.h

@@ -270,9 +270,9 @@ inline PyThreadState *get_thread_state_unchecked() {
 
 /// We use this count to figure out if there are multiple sub-interpreters currently present.
 /// Might be read/written from multiple interpreters in multiple threads at the same time with no
-/// syncronization. This must never decrease while any interpreter may be running in any thread!
-inline std::atomic<int64_t> &get_interpreter_count() {
-    static std::atomic<int64_t> counter(0);
+/// synchronization. This must never decrease while any interpreter may be running in any thread!
+inline std::atomic<int> &get_interpreter_count() {
+    static std::atomic<int> counter(0);
     return counter;
 }
 
@@ -444,25 +444,9 @@ inline uint64_t round_up_to_next_pow2(uint64_t x) {
 }
 
 template <typename InternalsType>
-inline InternalsType **find_or_create_internals_pp(char const *state_dict_key) {
-#if defined(PYBIND11_SIMPLE_GIL_MANAGEMENT)
-    gil_scoped_acquire gil;
-#else
-    // Ensure that the GIL is held since we will need to make Python calls.
-    // Cannot use py::gil_scoped_acquire here since that constructor calls get_internals.
-    struct gil_scoped_acquire_local {
-        gil_scoped_acquire_local() : state(PyGILState_Ensure()) {}
-        gil_scoped_acquire_local(const gil_scoped_acquire_local &) = delete;
-        gil_scoped_acquire_local &operator=(const gil_scoped_acquire_local &) = delete;
-        ~gil_scoped_acquire_local() { PyGILState_Release(state); }
-        const PyGILState_STATE state;
-    } gil;
-#endif
-
-    error_scope err_scope;
-
+inline InternalsType **find_internals_pp(char const *state_dict_key) {
     dict state_dict = get_python_state_dict();
-    object internals_obj
+    auto internals_obj
         = reinterpret_steal<object>(dict_getitemstringref(state_dict.ptr(), state_dict_key));
     if (internals_obj) {
         void *raw_ptr = PyCapsule_GetPointer(internals_obj.ptr(), /*name=*/nullptr);
@@ -472,21 +456,38 @@ inline InternalsType **find_or_create_internals_pp(char const *state_dict_key) {
             return reinterpret_cast<InternalsType **>(raw_ptr);
         }
     }
-
-    auto pp = new InternalsType *(nullptr);
-    state_dict[state_dict_key] = capsule(reinterpret_cast<void *>(pp));
-    return pp;
+    return nullptr;
 }
 
+#if defined(PYBIND11_SIMPLE_GIL_MANAGEMENT)
+using simple_gil_scoped_acquire = gil_scoped_acquire;
+#else
+// Cannot use py::gil_scoped_acquire inside get_internals since that calls get_internals.
+struct simple_gil_scoped_acquire {
+    simple_gil_scoped_acquire() : state(PyGILState_Ensure()) {}
+    simple_gil_scoped_acquire(const simple_gil_scoped_acquire &) = delete;
+    simple_gil_scoped_acquire &operator=(const simple_gil_scoped_acquire &) = delete;
+    simple_gil_scoped_acquire(simple_gil_scoped_acquire &&) = delete;
+    simple_gil_scoped_acquire &operator=(simple_gil_scoped_acquire &&) = delete;
+    ~simple_gil_scoped_acquire() { PyGILState_Release(state); }
+    const PyGILState_STATE state;
+};
+#endif
+
 /// Return a reference to the current `internals` data
 PYBIND11_NOINLINE internals &get_internals() {
     auto **&internals_pp = get_internals_pp();
     if (internals_pp && *internals_pp) {
         return **internals_pp;
     }
 
-    internals_pp = find_or_create_internals_pp<internals>(PYBIND11_INTERNALS_ID);
-    if (*internals_pp) {
+    simple_gil_scoped_acquire gil;
+
+    error_scope err_scope;
+
+    internals_pp = find_internals_pp<internals>(PYBIND11_INTERNALS_ID);
+
+    if (internals_pp && *internals_pp) {
         // We loaded the internals through `state_dict`, which means that our `error_already_set`
         // and `builtin_exception` may be different local classes than the ones set up in the
         // initial exception translator, below, so add another for our local exception classes.
@@ -503,10 +504,16 @@ PYBIND11_NOINLINE internals &get_internals() {
         }
 #endif
     } else {
+        if (!internals_pp) {
+            internals_pp = new internals *(nullptr);
+            dict state = get_python_state_dict();
+            state[PYBIND11_INTERNALS_ID] = capsule(reinterpret_cast<void *>(internals_pp));
+        }
+
         auto *&internals_ptr = *internals_pp;
         internals_ptr = new internals();
 
-        PyThreadState *tstate = get_thread_state_unchecked();
+        PyThreadState *tstate = PyThreadState_Get();
         // NOLINTNEXTLINE(bugprone-assignment-in-if-condition)
         if (!PYBIND11_TLS_KEY_CREATE(internals_ptr->tstate)) {
             pybind11_fail("get_internals: could not successfully initialize the tstate TSS key!");
@@ -572,24 +579,37 @@ inline local_internals **&get_local_internals_pp() {
     return s_internals_pp;
 }
 
+inline char const *get_local_internals_id() {
+    // we create a unique value at first run which is based on a pointer to a (non-thread_local)
+    // static value in this function, then multiple loaded modules using this code will still each
+    // have a unique key.
+    static const std::string this_module_idstr
+        = PYBIND11_MODULE_LOCAL_ID
+          + std::to_string(reinterpret_cast<uintptr_t>(&this_module_idstr));
+    return this_module_idstr.c_str();
+}
+
 /// Works like `get_internals`, but for things which are locally registered.
 inline local_internals &get_local_internals() {
     auto **&local_internals_pp = get_local_internals_pp();
     if (local_internals_pp && *local_internals_pp) {
         return **local_internals_pp;
     }
 
-    // we create a unique value at first run which is based on a pointer to a (non-thread_local)
-    // static value in this function, then multiple loaded modules using this code will still each
-    // have a unique key.
-    static const std::string this_module_idstr
-        = PYBIND11_MODULE_LOCAL_ID
-          + std::to_string(reinterpret_cast<uintptr_t>(&this_module_idstr));
+    simple_gil_scoped_acquire gil;
 
-    local_internals_pp = find_or_create_internals_pp<local_internals>(this_module_idstr.c_str());
+    error_scope err_scope;
+    
+    local_internals_pp = find_internals_pp<local_internals>(get_local_internals_id());
+    if (!local_internals_pp) {
+        local_internals_pp = new local_internals *(nullptr);
+        dict state = get_python_state_dict();
+        state[get_local_internals_id()] = capsule(reinterpret_cast<void *>(local_internals_pp));
+    }
     if (!*local_internals_pp) {
         *local_internals_pp = new local_internals();
     }
+
     return **local_internals_pp;
 }
 

include/pybind11/embed.h

@@ -236,21 +236,23 @@ inline void finalize_interpreter() {
     // Get the internals pointer (without creating it if it doesn't exist).  It's possible for the
     // internals to be created during Py_Finalize() (e.g. if a py::capsule calls `get_internals()`
     // during destruction), so we get the pointer-pointer here and check it after Py_Finalize().
-    auto **&internals_pp = detail::get_internals_pp();
+    auto **&internals_ptr_ptr = detail::get_internals_pp();
+    internals_ptr_ptr = detail::find_internals_pp<detail::internals>(PYBIND11_INTERNALS_ID);
     auto **&local_internals_pp = detail::get_local_internals_pp();
+    local_internals_pp = detail::find_internals_pp<detail::local_internals>(detail::get_local_internals_id());
 
     Py_Finalize();
 
-    if (internals_pp) {
-        delete *internals_pp;
-        *internals_pp = nullptr;
+    if (internals_ptr_ptr) {
+        delete *internals_ptr_ptr;
+        *internals_ptr_ptr = nullptr;
     }
 
     // Local internals contains data managed by the current interpreter, so we must clear them to
     // avoid undefined behaviors when initializing another interpreter
-    if (local_internals_pp) {
+    if (local_internals_pp && *local_internals_pp) {
         delete *local_internals_pp;
-        local_internals_pp = nullptr;
+        *local_internals_pp = nullptr;
     }
 
     detail::get_interpreter_count() = 0;

tests/test_embed/test_interpreter.cpp

@@ -256,8 +256,8 @@ TEST_CASE("Add program dir to path using PyConfig") {
 #endif
 
 bool has_state_dict_internals_obj() {
-    return bool(
-        py::detail::get_internals_obj_from_state_dict(py::detail::get_python_state_dict()));
+    py::dict state = py::detail::get_python_state_dict();
+    return state.contains(PYBIND11_INTERNALS_ID);
 }
 
 bool has_pybind11_internals_static() {
@@ -308,6 +308,7 @@ TEST_CASE("Restart the interpreter") {
     REQUIRE_FALSE(ran);
     py::finalize_interpreter();
     REQUIRE(ran);
+    REQUIRE_FALSE(has_pybind11_internals_static());
     py::initialize_interpreter();
     REQUIRE_FALSE(has_state_dict_internals_obj());
     REQUIRE_FALSE(has_pybind11_internals_static());
@@ -340,15 +341,16 @@ TEST_CASE("Subinterpreter") {
     auto *main_tstate = PyThreadState_Get();
     auto *sub_tstate = Py_NewInterpreter();
 
+    py::detail::get_interpreter_count()++;
+
     // Subinterpreters get their own copy of builtins.
     REQUIRE_FALSE(has_state_dict_internals_obj());
 
 #if defined(PYBIND11_SUBINTERPRETER_SUPPORT) && PY_VERSION_HEX >= 0x030C0000
     // internals hasn't been populated yet, but will be different for the subinterpreter
     REQUIRE_FALSE(has_pybind11_internals_static());
 
-    py::list sys_path = py::module_::import("sys").attr("path");
-    sys_path.append(py::str("."));
+    py::list(py::module_::import("sys").attr("path")).append(py::str("."));
 
     auto ext_int = py::module_::import("external_module").attr("internals_at")().cast<uintptr_t>();
     py::detail::get_internals();
@@ -374,11 +376,13 @@ TEST_CASE("Subinterpreter") {
 
     // Restore main interpreter.
     Py_EndInterpreter(sub_tstate);
+    py::detail::get_interpreter_count() = 1;
     PyThreadState_Swap(main_tstate);
 
     REQUIRE(py::hasattr(py::module_::import("__main__"), "main_tag"));
     REQUIRE(py::hasattr(py::module_::import("widget_module"), "extension_module_tag"));
     REQUIRE(has_state_dict_internals_obj());
+
 }
 
 TEST_CASE("Execution frame") {