Directly check if/which interpreter is active before doing CLEAR in destructor

[b-pass]

Description

Fix of the fix from #5958 ... Py_IsFinalizing and Py_IsInitialized only apply to the main interpreter.

[rwgk]

@b-pass, I worked on a "best of both" version under PR #5966 (commit 8756fb2 there)

You can get the suggested diff like this:

git diff 08ea6ca4e64229e6cf234c8be5b6c82320e37860..8756fb2543d87df8c294fbb57db0dede3654089b

Copy-pasting for easy reference:

diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h
index 2811077e..2cd3ea4a 100644
--- a/include/pybind11/detail/internals.h
+++ b/include/pybind11/detail/internals.h
@@ -337,13 +337,24 @@ struct internals {
     internals(internals &&other) = delete;
     internals &operator=(const internals &other) = delete;
     internals &operator=(internals &&other) = delete;
+
+    void leak_detach() noexcept {
+        // Used when internals must be destroyed after interpreter teardown.
+        // Avoid touching the Python C-API by clearing pointers only.
+        instance_base = nullptr;
+        default_metaclass = nullptr;
+        static_property_type = nullptr;
+        istate = nullptr;
+    }
+
     ~internals() {
         // Normally this destructor runs during interpreter finalization and it may DECREF things.
         // In odd finalization scenarios it might end up running after the interpreter has
         // completely shut down, In that case, we should not decref these objects because pymalloc
         // is gone.  This also applies across sub-interpreters, we should only DECREF when the
         // original owning interpreter is active.
-        if (get_interpreter_state_unchecked() == istate) {
+        auto *cur_istate = get_interpreter_state_unchecked();
+        if (cur_istate && cur_istate == istate) {
             Py_CLEAR(instance_base);
             Py_CLEAR(default_metaclass);
             Py_CLEAR(static_property_type);
@@ -369,13 +380,20 @@ struct local_internals {
     PyTypeObject *function_record_py_type = nullptr;
     PyInterpreterState *istate = nullptr;
 
+    void leak_detach() noexcept {
+        // Used when local internals must be destroyed after interpreter teardown.
+        function_record_py_type = nullptr;
+        istate = nullptr;
+    }
+
     ~local_internals() {
         // Normally this destructor runs during interpreter finalization and it may DECREF things.
         // In odd finalization scenarios it might end up running after the interpreter has
         // completely shut down, In that case, we should not decref these objects because pymalloc
         // is gone.  This also applies across sub-interpreters, we should only DECREF when the
         // original owning interpreter is active.
-        if (get_interpreter_state_unchecked() == istate) {
+        auto *cur_istate = get_interpreter_state_unchecked();
+        if (cur_istate && cur_istate == istate) {
             Py_CLEAR(function_record_py_type);
         }
     }
@@ -711,13 +729,24 @@ public:
             // this could be called without an active interpreter, just use what was cached
             if (!tstate || tstate->interp == last_istate_tls()) {
                 auto tpp = internals_p_tls();
-
+                if (tpp && tpp->get()) {
+                    auto *cur_istate = get_interpreter_state_unchecked();
+                    if (!cur_istate || cur_istate != tpp->get()->istate) {
+                        tpp->get()->leak_detach();
+                    }
+                }
                 delete tpp;
             }
             unref();
             return;
         }
 #endif
+        if (internals_singleton_pp_ && internals_singleton_pp_->get()) {
+            auto *cur_istate = get_interpreter_state_unchecked();
+            if (!cur_istate || cur_istate != internals_singleton_pp_->get()->istate) {
+                internals_singleton_pp_->get()->leak_detach();
+            }
+        }
         delete internals_singleton_pp_;
         unref();
     }

[rwgk]

Looks good to me.

Do you want to leave the leak_detach() change for a follow-on PR? Or do you think they are not needed?

[oremanj]

Looks great, thanks!

[b-pass]

Do you want to leave the leak_detach() change for a follow-on PR? Or do you think they are not needed?

It looks to me like leak_detach isn't needed any more, since the check that calls it now happens within the destructor. I think it's better to have it in the destructor because it keeps the logic for whether to clear right next to the code that calls clear, rather than split up between the pp_manager and the internals itself.