Skip to content

Commit e36b09d

Browse files
Vipul-CariappaVipul-Cariappa
committed
[cppyy] fix usage of Python GIL to support threading
1 parent 34be61d commit e36b09d

File tree

3 files changed

+31
-39
lines changed

3 files changed

+31
-39
lines changed

‎bindings/pyroot/cppyy/CPyCppyy/src/DispatchPtr.cxx‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,23 +10,25 @@
1010
//-----------------------------------------------------------------------------
1111
PyObject* CPyCppyy::DispatchPtr::Get(bool borrowed) const
1212
{
13+
PyGILState_STATE state = PyGILState_Ensure();
14+
PyObject* result = nullptr;
1315
if (fPyHardRef) {
1416
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);
2222
}
2323
}
24-
return nullptr;
24+
PyGILState_Release(state);
25+
return result;
2526
}
2627

2728
//-----------------------------------------------------------------------------
2829
CPyCppyy::DispatchPtr::DispatchPtr(PyObject* pyobj, bool strong) : fPyHardRef(nullptr)
2930
{
31+
PyGILState_STATE state = PyGILState_Ensure();
3032
if (strong) {
3133
Py_INCREF(pyobj);
3234
fPyHardRef = pyobj;
@@ -36,15 +38,18 @@ CPyCppyy::DispatchPtr::DispatchPtr(PyObject* pyobj, bool strong) : fPyHardRef(nu
3638
fPyWeakRef = PyWeakref_NewRef(pyobj, nullptr);
3739
}
3840
((CPPInstance*)pyobj)->SetDispatchPtr(this);
41+
PyGILState_Release(state);
3942
}
4043

4144
//-----------------------------------------------------------------------------
4245
CPyCppyy::DispatchPtr::DispatchPtr(const DispatchPtr& other, void* cppinst) : fPyWeakRef(nullptr)
4346
{
47+
PyGILState_STATE state = PyGILState_Ensure();
4448
PyObject* pyobj = other.Get(false /* not borrowed */);
4549
fPyHardRef = pyobj ? (PyObject*)((CPPInstance*)pyobj)->Copy(cppinst) : nullptr;
4650
if (fPyHardRef) ((CPPInstance*)fPyHardRef)->SetDispatchPtr(this);
4751
Py_XDECREF(pyobj);
52+
PyGILState_Release(state);
4853
}
4954

5055
//-----------------------------------------------------------------------------
@@ -53,6 +58,7 @@ CPyCppyy::DispatchPtr::~DispatchPtr() {
5358
// of a dispatcher intermediate, then this delete is from the C++ side, and Python
5459
// is "notified" by nulling out the reference and an exception will be raised on
5560
// continued access
61+
PyGILState_STATE state = PyGILState_Ensure();
5662
if (fPyWeakRef) {
5763
PyObject* pyobj = CPyCppyy_GetWeakRef(fPyWeakRef);
5864
if (pyobj && ((CPPScope*)Py_TYPE(pyobj))->fFlags & CPPScope::kIsPython)
@@ -63,11 +69,13 @@ CPyCppyy::DispatchPtr::~DispatchPtr() {
6369
((CPPInstance*)fPyHardRef)->GetObjectRaw() = nullptr;
6470
Py_DECREF(fPyHardRef);
6571
}
72+
PyGILState_Release(state);
6673
}
6774

6875
//-----------------------------------------------------------------------------
6976
CPyCppyy::DispatchPtr& CPyCppyy::DispatchPtr::assign(const DispatchPtr& other, void* cppinst)
7077
{
78+
PyGILState_STATE state = PyGILState_Ensure();
7179
if (this != &other) {
7280
Py_XDECREF(fPyWeakRef); fPyWeakRef = nullptr;
7381
Py_XDECREF(fPyHardRef);
@@ -76,6 +84,7 @@ CPyCppyy::DispatchPtr& CPyCppyy::DispatchPtr::assign(const DispatchPtr& other, v
7684
if (fPyHardRef) ((CPPInstance*)fPyHardRef)->SetDispatchPtr(this);
7785
Py_XDECREF(pyobj);
7886
}
87+
PyGILState_Release(state);
7988
return *this;
8089
}
8190

@@ -93,8 +102,10 @@ void CPyCppyy::DispatchPtr::PythonOwns()
93102
void CPyCppyy::DispatchPtr::CppOwns()
94103
{
95104
// C++ maintains the hardref, keeping the PyObject alive w/o outstanding ref
105+
PyGILState_STATE state = PyGILState_Ensure();
96106
if (fPyWeakRef) {
97107
fPyHardRef = CPyCppyy_GetWeakRef(fPyWeakRef);
98108
Py_DECREF(fPyWeakRef); fPyWeakRef = nullptr;
99109
}
110+
PyGILState_Release(state);
100111
}

‎bindings/pyroot/cppyy/CPyCppyy/src/Dispatcher.cxx‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@ static inline void InjectMethod(Cppyy::TCppMethod_t method, const std::string& m
4141
// possible crash
4242
code << " PyObject* iself = (PyObject*)_internal_self;\n"
4343
" if (!iself || iself == Py_None) {\n"
44+
" PyGILState_STATE state = PyGILState_Ensure();\n"
4445
" PyErr_Warn(PyExc_RuntimeWarning, (char*)\"Call attempted on deleted python-side proxy\");\n"
46+
" PyGILState_Release(state);\n"
4547
" return";
4648
if (retType != "void") {
4749
if (retType.back() != '*')
@@ -50,12 +52,13 @@ static inline void InjectMethod(Cppyy::TCppMethod_t method, const std::string& m
5052
code << " nullptr";
5153
}
5254
code << ";\n"
53-
" }\n"
54-
" Py_INCREF(iself);\n";
55+
" }\n";
5556

5657
// start actual function body
5758
Utility::ConstructCallbackPreamble(retType, argtypes, code);
5859

60+
code << " Py_INCREF(iself);\n";
61+
5962
// perform actual method call
6063
#if PY_VERSION_HEX < 0x03000000
6164
code << " PyObject* mtPyName = PyString_FromString(\"" << mtCppName << "\");\n" // TODO: intern?
@@ -238,6 +241,7 @@ bool CPyCppyy::InsertDispatcher(CPPScope* klass, PyObject* bases, PyObject* dct,
238241
// object goes before the C++ one, only __del__ is called)
239242
if (PyMapping_HasKeyString(dct, (char*)"__destruct__")) {
240243
code << " virtual ~" << derivedName << "() {\n"
244+
"PyGILState_STATE state = PyGILState_Ensure();\n"
241245
" PyObject* iself = (PyObject*)_internal_self;\n"
242246
" if (!iself || iself == Py_None)\n"
243247
" return;\n" // safe, as destructor always returns void
@@ -250,6 +254,7 @@ bool CPyCppyy::InsertDispatcher(CPPScope* klass, PyObject* bases, PyObject* dct,
250254
// magic C++ exception ...
251255
code << " if (!pyresult) PyErr_Print();\n"
252256
" else { Py_DECREF(pyresult); }\n"
257+
" PyGILState_Release(state);\n"
253258
" }\n";
254259
} else
255260
code << " virtual ~" << derivedName << "() {}\n";
@@ -450,8 +455,11 @@ bool CPyCppyy::InsertDispatcher(CPPScope* klass, PyObject* bases, PyObject* dct,
450455

451456
// provide an accessor to re-initialize after round-tripping from C++ (internal)
452457
code << "\n static PyObject* _get_dispatch(" << derivedName << "* inst) {\n"
458+
" PyGILState_STATE state = PyGILState_Ensure();\n"
453459
" PyObject* res = (PyObject*)inst->_internal_self;\n"
454-
" Py_XINCREF(res); return res;\n }";
460+
" Py_XINCREF(res);\n"
461+
" PyGILState_Release(state);\n"
462+
" return res;\n }";
455463

456464
// finish class declaration
457465
code << "};\n}";

‎bindings/pyroot/cppyy/cppyy/test/test_concurrent.py‎

Lines changed: 1 addition & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -110,41 +110,19 @@ def test04_cpp_threading_with_exceptions(self):
110110
};
111111
112112
struct worker {
113-
worker(consumer* c) : cons(c) {
114-
// Get the main interpreter state state to spawn new thread states
115-
PyThreadState* state = PyThreadState_Get();
116-
interpreterState = state->interp;
117-
}
113+
worker(consumer* c) : cons(c) {}
118114
~worker() { wait(); }
119115
120116
void start() {
121117
t = std::thread([this] {
122118
int counter = 0;
123-
124-
// Each thread needs a Python state object
125-
// Instead of using the higher-level PyGILState_Ensure and
126-
// PyGILState_Release functions, use the PyThreadState API
127-
// directly so that we only need to create one single
128-
// PyThreadState that can be restored and released in the
129-
// "hot loop".
130-
PyThreadState *pystate = PyThreadState_New(this->interpreterState);
131-
132119
while (counter++ < 10)
133120
try {
134-
PyEval_RestoreThread(pystate);
135121
cons->process(counter);
136-
PyEval_SaveThread();
137122
} catch (CPyCppyy::PyException& e) {
138123
err_msg = e.what();
139-
PyEval_SaveThread();
140124
return;
141125
}
142-
143-
PyEval_RestoreThread(pystate);
144-
PyThreadState_Clear(pystate);
145-
PyEval_SaveThread();
146-
147-
PyThreadState_Delete(pystate);
148126
});
149127
}
150128
@@ -153,7 +131,6 @@ def test04_cpp_threading_with_exceptions(self):
153131
t.join();
154132
}
155133
156-
PyInterpreterState* interpreterState = nullptr;
157134
std::thread t;
158135
consumer* cons = nullptr;
159136
std::string err_msg;
@@ -229,11 +206,7 @@ def test05_float2d_callback(self):
229206
for (int i = 0; i < channels; ++i)
230207
data[i] = new float[samples];
231208
232-
// Set Python thread because we call back into Python
233-
PyGILState_STATE gstate;
234-
gstate = PyGILState_Ensure();
235209
p.process(data, channels, samples);
236-
PyGILState_Release(gstate);
237210
238211
for (int i = 0; i < channels; ++i)
239212
delete[] data[i];

0 commit comments

Comments
 (0)