[cppyy] fix usage of Python GIL to support threading

Vipul-Cariappa · guitargeek · commit 12c0d699baee · 2025-07-25T09:37:51.000+02:00
diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/DispatchPtr.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/DispatchPtr.cxx
@@ -10,23 +10,25 @@
 //-----------------------------------------------------------------------------
 PyObject* CPyCppyy::DispatchPtr::Get(bool borrowed) const
 {
+    PyGILState_STATE state = PyGILState_Ensure();
+    PyObject* result = nullptr;
     if (fPyHardRef) {
         if (!borrowed) Py_INCREF(fPyHardRef);
-        return fPyHardRef;
-    }
-    if (fPyWeakRef) {
-        PyObject* disp = CPyCppyy_GetWeakRef(fPyWeakRef);
-        if (disp) {               // dispatcher object disappeared?
-            if (borrowed) Py_DECREF(disp);
-            return disp;
+        result = fPyHardRef;
+    } else if (fPyWeakRef) {
+        result = CPyCppyy_GetWeakRef(fPyWeakRef);
+        if (result) {               // dispatcher object disappeared?
+            if (borrowed) Py_DECREF(result);
         }
     }
-    return nullptr;
+    PyGILState_Release(state);
+    return result;
 }
 
 //-----------------------------------------------------------------------------
 CPyCppyy::DispatchPtr::DispatchPtr(PyObject* pyobj, bool strong) : fPyHardRef(nullptr)
 {
+    PyGILState_STATE state = PyGILState_Ensure();
     if (strong) {
         Py_INCREF(pyobj);
         fPyHardRef = pyobj;
@@ -36,15 +38,18 @@ CPyCppyy::DispatchPtr::DispatchPtr(PyObject* pyobj, bool strong) : fPyHardRef(nu
         fPyWeakRef = PyWeakref_NewRef(pyobj, nullptr);
     }
     ((CPPInstance*)pyobj)->SetDispatchPtr(this);
+    PyGILState_Release(state);
 }
 
 //-----------------------------------------------------------------------------
 CPyCppyy::DispatchPtr::DispatchPtr(const DispatchPtr& other, void* cppinst) : fPyWeakRef(nullptr)
 {
+    PyGILState_STATE state = PyGILState_Ensure();
     PyObject* pyobj = other.Get(false /* not borrowed */);
     fPyHardRef = pyobj ? (PyObject*)((CPPInstance*)pyobj)->Copy(cppinst) : nullptr;
     if (fPyHardRef) ((CPPInstance*)fPyHardRef)->SetDispatchPtr(this);
     Py_XDECREF(pyobj);
+    PyGILState_Release(state);
 }
 
 //-----------------------------------------------------------------------------
@@ -53,6 +58,7 @@ CPyCppyy::DispatchPtr::~DispatchPtr() {
 // of a dispatcher intermediate, then this delete is from the C++ side, and Python
 // is "notified" by nulling out the reference and an exception will be raised on
 // continued access
+    PyGILState_STATE state = PyGILState_Ensure();
     if (fPyWeakRef) {
         PyObject* pyobj = CPyCppyy_GetWeakRef(fPyWeakRef);
         if (pyobj && ((CPPScope*)Py_TYPE(pyobj))->fFlags & CPPScope::kIsPython)
@@ -63,11 +69,13 @@ CPyCppyy::DispatchPtr::~DispatchPtr() {
         ((CPPInstance*)fPyHardRef)->GetObjectRaw() = nullptr;
         Py_DECREF(fPyHardRef);
     }
+    PyGILState_Release(state);
 }
 
 //-----------------------------------------------------------------------------
 CPyCppyy::DispatchPtr& CPyCppyy::DispatchPtr::assign(const DispatchPtr& other, void* cppinst)
 {
+    PyGILState_STATE state = PyGILState_Ensure();
     if (this != &other) {
         Py_XDECREF(fPyWeakRef); fPyWeakRef = nullptr;
         Py_XDECREF(fPyHardRef);
@@ -76,6 +84,7 @@ CPyCppyy::DispatchPtr& CPyCppyy::DispatchPtr::assign(const DispatchPtr& other, v
         if (fPyHardRef) ((CPPInstance*)fPyHardRef)->SetDispatchPtr(this);
         Py_XDECREF(pyobj);
     }
+    PyGILState_Release(state);
     return *this;
 }
 
@@ -93,8 +102,10 @@ void CPyCppyy::DispatchPtr::PythonOwns()
 void CPyCppyy::DispatchPtr::CppOwns()
 {
 // C++ maintains the hardref, keeping the PyObject alive w/o outstanding ref
+    PyGILState_STATE state = PyGILState_Ensure();
     if (fPyWeakRef) {
         fPyHardRef = CPyCppyy_GetWeakRef(fPyWeakRef);
         Py_DECREF(fPyWeakRef); fPyWeakRef = nullptr;
     }
+    PyGILState_Release(state);
 }
diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/Dispatcher.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/Dispatcher.cxx
@@ -41,7 +41,9 @@ static inline void InjectMethod(Cppyy::TCppMethod_t method, const std::string& m
 // possible crash
     code << "    PyObject* iself = (PyObject*)_internal_self;\n"
             "    if (!iself || iself == Py_None) {\n"
+            "      PyGILState_STATE state = PyGILState_Ensure();\n"
             "      PyErr_Warn(PyExc_RuntimeWarning, (char*)\"Call attempted on deleted python-side proxy\");\n"
+            "      PyGILState_Release(state);\n"
             "      return";
     if (retType != "void") {
         if (retType.back() != '*')
@@ -50,12 +52,13 @@ static inline void InjectMethod(Cppyy::TCppMethod_t method, const std::string& m
             code << " nullptr";
     }
     code << ";\n"
-            "    }\n"
-            "    Py_INCREF(iself);\n";
+            "    }\n";
 
 // start actual function body
     Utility::ConstructCallbackPreamble(retType, argtypes, code);
 
+    code << "    Py_INCREF(iself);\n";
+
 // perform actual method call
 #if PY_VERSION_HEX < 0x03000000
     code << "    PyObject* mtPyName = PyString_FromString(\"" << mtCppName << "\");\n" // TODO: intern?
@@ -238,6 +241,7 @@ bool CPyCppyy::InsertDispatcher(CPPScope* klass, PyObject* bases, PyObject* dct,
 // object goes before the C++ one, only __del__ is called)
     if (PyMapping_HasKeyString(dct, (char*)"__destruct__")) {
         code << "  virtual ~" << derivedName << "() {\n"
+                "PyGILState_STATE state = PyGILState_Ensure();\n"
                 "    PyObject* iself = (PyObject*)_internal_self;\n"
                 "    if (!iself || iself == Py_None)\n"
                 "      return;\n"      // safe, as destructor always returns void
@@ -250,6 +254,7 @@ bool CPyCppyy::InsertDispatcher(CPPScope* klass, PyObject* bases, PyObject* dct,
     // magic C++ exception ...
         code << "      if (!pyresult) PyErr_Print();\n"
                 "      else { Py_DECREF(pyresult); }\n"
+                "      PyGILState_Release(state);\n"
                 "  }\n";
     } else
         code << "  virtual ~" << derivedName << "() {}\n";
@@ -450,8 +455,11 @@ bool CPyCppyy::InsertDispatcher(CPPScope* klass, PyObject* bases, PyObject* dct,
 
 // provide an accessor to re-initialize after round-tripping from C++ (internal)
     code << "\n  static PyObject* _get_dispatch(" << derivedName << "* inst) {\n"
+            "    PyGILState_STATE state = PyGILState_Ensure();\n"
             "    PyObject* res = (PyObject*)inst->_internal_self;\n"
-            "    Py_XINCREF(res); return res;\n  }";
+            "    Py_XINCREF(res);\n"
+            "    PyGILState_Release(state);\n"
+            "    return res;\n  }";
 
 // finish class declaration
     code << "};\n}";
diff --git a/bindings/pyroot/cppyy/cppyy/test/test_concurrent.py b/bindings/pyroot/cppyy/cppyy/test/test_concurrent.py
@@ -110,41 +110,19 @@ def test04_cpp_threading_with_exceptions(self):
         };
 
         struct worker {
-            worker(consumer* c) : cons(c) {
-                // Get the main interpreter state state to spawn new thread states
-                PyThreadState* state = PyThreadState_Get();
-                interpreterState = state->interp;
-            }
+            worker(consumer* c) : cons(c) {}
             ~worker() { wait(); }
 
             void start() {
                 t = std::thread([this] {
                     int counter = 0;
-
-                    // Each thread needs a Python state object
-                    // Instead of using the higher-level PyGILState_Ensure and
-                    // PyGILState_Release functions, use the PyThreadState API
-                    // directly so that we only need to create one single
-                    // PyThreadState that can be restored and released in the
-                    // "hot loop".
-                    PyThreadState *pystate = PyThreadState_New(this->interpreterState);
-
                     while (counter++ < 10)
                         try {
-                            PyEval_RestoreThread(pystate);
                             cons->process(counter);
-                            PyEval_SaveThread();
                         } catch (CPyCppyy::PyException& e) {
                             err_msg = e.what();
-                            PyEval_SaveThread();
                             return;
                         }
-
-                    PyEval_RestoreThread(pystate);
-                    PyThreadState_Clear(pystate);
-                    PyEval_SaveThread();
-
-                    PyThreadState_Delete(pystate);
                 });
             }
 
@@ -153,7 +131,6 @@ def test04_cpp_threading_with_exceptions(self):
                     t.join();
             }
 
-            PyInterpreterState* interpreterState = nullptr;
             std::thread t;
             consumer* cons = nullptr;
             std::string err_msg;
@@ -229,11 +206,7 @@ def test05_float2d_callback(self):
                 for (int i = 0; i < channels; ++i)
                     data[i] = new float[samples];
 
-                // Set Python thread because we call back into Python
-                PyGILState_STATE gstate;
-                gstate = PyGILState_Ensure();
                 p.process(data, channels, samples);
-                PyGILState_Release(gstate);
 
                 for (int i = 0; i < channels; ++i)
                     delete[] data[i];

Original file line number	Diff line number	Diff line change
`@@ -10,23 +10,25 @@`
`10`	`10`	`//-----------------------------------------------------------------------------`
`11`	`11`	`PyObject* CPyCppyy::DispatchPtr::Get(bool borrowed) const`
`12`	`12`	`{`
	`13`	`+ PyGILState_STATE state = PyGILState_Ensure();`
	`14`	`+ PyObject* result = nullptr;`
`13`	`15`	`if (fPyHardRef) {`
`14`	`16`	`if (!borrowed) Py_INCREF(fPyHardRef);`
`15`		`- return fPyHardRef;`
`16`		`- }`
`17`		`- if (fPyWeakRef) {`
`18`		`- PyObject* disp = CPyCppyy_GetWeakRef(fPyWeakRef);`
`19`		`- if (disp) { // dispatcher object disappeared?`
`20`		`- if (borrowed) Py_DECREF(disp);`
`21`		`- return disp;`
	`17`	`+ result = fPyHardRef;`
	`18`	`+ } else if (fPyWeakRef) {`
	`19`	`+ result = CPyCppyy_GetWeakRef(fPyWeakRef);`
	`20`	`+ if (result) { // dispatcher object disappeared?`
	`21`	`+ if (borrowed) Py_DECREF(result);`
`22`	`22`	`}`
`23`	`23`	`}`
`24`		`- return nullptr;`
	`24`	`+ PyGILState_Release(state);`
	`25`	`+ return result;`
`25`	`26`	`}`
`26`	`27`
`27`	`28`	`//-----------------------------------------------------------------------------`
`28`	`29`	`CPyCppyy::DispatchPtr::DispatchPtr(PyObject* pyobj, bool strong) : fPyHardRef(nullptr)`
`29`	`30`	`{`
	`31`	`+ PyGILState_STATE state = PyGILState_Ensure();`
`30`	`32`	`if (strong) {`
`31`	`33`	`Py_INCREF(pyobj);`
`32`	`34`	`fPyHardRef = pyobj;`
`@@ -36,15 +38,18 @@ CPyCppyy::DispatchPtr::DispatchPtr(PyObject* pyobj, bool strong) : fPyHardRef(nu`
`36`	`38`	`fPyWeakRef = PyWeakref_NewRef(pyobj, nullptr);`
`37`	`39`	`}`
`38`	`40`	`((CPPInstance*)pyobj)->SetDispatchPtr(this);`
	`41`	`+ PyGILState_Release(state);`
`39`	`42`	`}`
`40`	`43`
`41`	`44`	`//-----------------------------------------------------------------------------`
`42`	`45`	`CPyCppyy::DispatchPtr::DispatchPtr(const DispatchPtr& other, void* cppinst) : fPyWeakRef(nullptr)`
`43`	`46`	`{`
	`47`	`+ PyGILState_STATE state = PyGILState_Ensure();`
`44`	`48`	`PyObject* pyobj = other.Get(false /* not borrowed */);`
`45`	`49`	`fPyHardRef = pyobj ? (PyObject)((CPPInstance)pyobj)->Copy(cppinst) : nullptr;`
`46`	`50`	`if (fPyHardRef) ((CPPInstance*)fPyHardRef)->SetDispatchPtr(this);`
`47`	`51`	`Py_XDECREF(pyobj);`
	`52`	`+ PyGILState_Release(state);`
`48`	`53`	`}`
`49`	`54`
`50`	`55`	`//-----------------------------------------------------------------------------`
`@@ -53,6 +58,7 @@ CPyCppyy::DispatchPtr::~DispatchPtr() {`
`53`	`58`	`// of a dispatcher intermediate, then this delete is from the C++ side, and Python`
`54`	`59`	`// is "notified" by nulling out the reference and an exception will be raised on`
`55`	`60`	`// continued access`
	`61`	`+ PyGILState_STATE state = PyGILState_Ensure();`
`56`	`62`	`if (fPyWeakRef) {`
`57`	`63`	`PyObject* pyobj = CPyCppyy_GetWeakRef(fPyWeakRef);`
`58`	`64`	`if (pyobj && ((CPPScope*)Py_TYPE(pyobj))->fFlags & CPPScope::kIsPython)`
`@@ -63,11 +69,13 @@ CPyCppyy::DispatchPtr::~DispatchPtr() {`
`63`	`69`	`((CPPInstance*)fPyHardRef)->GetObjectRaw() = nullptr;`
`64`	`70`	`Py_DECREF(fPyHardRef);`
`65`	`71`	`}`
	`72`	`+ PyGILState_Release(state);`
`66`	`73`	`}`
`67`	`74`
`68`	`75`	`//-----------------------------------------------------------------------------`
`69`	`76`	`CPyCppyy::DispatchPtr& CPyCppyy::DispatchPtr::assign(const DispatchPtr& other, void* cppinst)`
`70`	`77`	`{`
	`78`	`+ PyGILState_STATE state = PyGILState_Ensure();`
`71`	`79`	`if (this != &other) {`
`72`	`80`	`Py_XDECREF(fPyWeakRef); fPyWeakRef = nullptr;`
`73`	`81`	`Py_XDECREF(fPyHardRef);`
`@@ -76,6 +84,7 @@ CPyCppyy::DispatchPtr& CPyCppyy::DispatchPtr::assign(const DispatchPtr& other, v`
`76`	`84`	`if (fPyHardRef) ((CPPInstance*)fPyHardRef)->SetDispatchPtr(this);`
`77`	`85`	`Py_XDECREF(pyobj);`
`78`	`86`	`}`
	`87`	`+ PyGILState_Release(state);`
`79`	`88`	`return *this;`
`80`	`89`	`}`
`81`	`90`
`@@ -93,8 +102,10 @@ void CPyCppyy::DispatchPtr::PythonOwns()`
`93`	`102`	`void CPyCppyy::DispatchPtr::CppOwns()`
`94`	`103`	`{`
`95`	`104`	`// C++ maintains the hardref, keeping the PyObject alive w/o outstanding ref`
	`105`	`+ PyGILState_STATE state = PyGILState_Ensure();`
`96`	`106`	`if (fPyWeakRef) {`
`97`	`107`	`fPyHardRef = CPyCppyy_GetWeakRef(fPyWeakRef);`
`98`	`108`	`Py_DECREF(fPyWeakRef); fPyWeakRef = nullptr;`
`99`	`109`	`}`
	`110`	`+ PyGILState_Release(state);`
`100`	`111`	`}`