From 60f02f5f663829c9fd2d7d52c03d4064129e38df Mon Sep 17 00:00:00 2001
From: Ethan Steinberg <ethan.steinberg@gmail.com>
Date: Fri, 30 Dec 2022 10:46:55 -0800
Subject: [PATCH] fix: improve the error reporting for inc_ref GIL failures
 (#4427)

* First

* Fixs

* Improve

* Additional assertions comment

* Improve docs
---
 docs/advanced/misc.rst     | 28 ++++++++++++++++++++++++++++
 include/pybind11/pytypes.h | 30 +++++++++++++++++++++++++-----
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/docs/advanced/misc.rst b/docs/advanced/misc.rst
index 35a6ebcd6..805ec838f 100644
--- a/docs/advanced/misc.rst
+++ b/docs/advanced/misc.rst
@@ -118,6 +118,34 @@ The ``call_go`` wrapper can also be simplified using the ``call_guard`` policy
     m.def("call_go", &call_go, py::call_guard<py::gil_scoped_release>());
 
 
+Common Sources Of Global Interpreter Lock Errors
+==================================================================
+
+Failing to properly hold the Global Interpreter Lock (GIL) is one of the
+more common sources of bugs within code that uses pybind11. If you are
+running into GIL related errors, we highly recommend you consult the
+following checklist.
+
+- Do you have any global variables that are pybind11 objects or invoke
+  pybind11 functions in either their constructor or destructor? You are generally
+  not allowed to invoke any Python function in a global static context. We recommend
+  using lazy initialization and then intentionally leaking at the end of the program.
+
+- Do you have any pybind11 objects that are members of other C++ structures? One
+  commonly overlooked requirement is that pybind11 objects have to increase their reference count
+  whenever their copy constructor is called. Thus, you need to be holding the GIL to invoke
+  the copy constructor of any C++ class that has a pybind11 member. This can sometimes be very
+  tricky to track for complicated programs Think carefully when you make a pybind11 object
+  a member in another struct.
+
+- C++ destructors that invoke Python functions can be particularly troublesome as
+  destructors can sometimes get invoked in weird and unexpected circumstances as a result
+  of exceptions.
+
+- You should try running your code in a debug build. That will enable additional assertions
+  within pybind11 that will throw exceptions on certain GIL handling errors
+  (reference counting operations).
+
 Binding sequence data types, iterators, the slicing protocol, etc.
 ==================================================================
 
diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h
index 56e0423ac..3660c180d 100644
--- a/include/pybind11/pytypes.h
+++ b/include/pybind11/pytypes.h
@@ -250,9 +250,9 @@ public:
 #ifdef PYBIND11_HANDLE_REF_DEBUG
         inc_ref_counter(1);
 #endif
-#if defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF)
+#ifdef PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF
         if (m_ptr != nullptr && !PyGILState_Check()) {
-            throw std::runtime_error("pybind11::handle::inc_ref() PyGILState_Check() failure.");
+            throw_gilstate_error("pybind11::handle::inc_ref()");
         }
 #endif
         Py_XINCREF(m_ptr);
@@ -265,9 +265,9 @@ public:
         this function automatically. Returns a reference to itself.
     \endrst */
     const handle &dec_ref() const & {
-#if defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF)
+#ifdef PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF
         if (m_ptr != nullptr && !PyGILState_Check()) {
-            throw std::runtime_error("pybind11::handle::dec_ref() PyGILState_Check() failure.");
+            throw_gilstate_error("pybind11::handle::dec_ref()");
         }
 #endif
         Py_XDECREF(m_ptr);
@@ -296,8 +296,28 @@ public:
 protected:
     PyObject *m_ptr = nullptr;
 
-#ifdef PYBIND11_HANDLE_REF_DEBUG
 private:
+#ifdef PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF
+    void throw_gilstate_error(const std::string &function_name) const {
+        fprintf(
+            stderr,
+            "%s is being called while the GIL is either not held or invalid. Please see "
+            "https://pybind11.readthedocs.io/en/stable/advanced/"
+            "misc.html#common-sources-of-global-interpreter-lock-errors for debugging advice.\n",
+            function_name.c_str());
+        fflush(stderr);
+        if (Py_TYPE(m_ptr)->tp_name != nullptr) {
+            fprintf(stderr,
+                    "The failing %s call was triggered on a %s object.\n",
+                    function_name.c_str(),
+                    Py_TYPE(m_ptr)->tp_name);
+            fflush(stderr);
+        }
+        throw std::runtime_error(function_name + " PyGILState_Check() failure.");
+    }
+#endif
+
+#ifdef PYBIND11_HANDLE_REF_DEBUG
     static std::size_t inc_ref_counter(std::size_t add) {
         thread_local std::size_t counter = 0;
         counter += add;