Skip to content

Commit 7f21de6

Browse files
committed
[Python] Don't exploit garbage collection hooks for memory relgulator
Looking at the CPPInstance, I noticed that is claims to support cycling garbage collection via the `Py_TPFLAGS_HAVE_GC` for not the right reasons. It doesn't stick at all to the contract explained in the docs: https://docs.python.org/3/c-api/gcsupport.html#c.PyObject_GC_Track The `tp_traverse` and `tp_clear` implementations actually do nothing with the C Python API, and the class also doesn't implement a custom `tp_alloc` and `tp_free`. I think the CPPInstance tries to exploit the python garbage collector to trigger its own memory regulator code path, but breaking some of the Python C API contracts on the way. This might be the reason for the spurious failures in the CI, which happen *between* two tests, hinting to garbage collector problems. I think it's particularly problematic that the CPPInstance never registers itself to the GC with `PyObject_GC_Track`, but then uses `PyObject_GC_UnTrack`. That probably brings the garbage collector in a bad state.
1 parent 3ab1d5c commit 7f21de6

File tree

1 file changed

+4
-16
lines changed

1 file changed

+4
-16
lines changed

bindings/pyroot/cppyy/CPyCppyy/src/CPPInstance.cxx

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -446,21 +446,10 @@ static CPPInstance* op_new(PyTypeObject* subtype, PyObject*, PyObject*)
446446
//----------------------------------------------------------------------------
447447
static void op_dealloc(CPPInstance* pyobj)
448448
{
449-
// Remove (Python-side) memory held by the object proxy.
450-
PyObject_GC_UnTrack((PyObject*)pyobj);
451449
op_dealloc_nofree(pyobj);
452-
PyObject_GC_Del((PyObject*)pyobj);
453-
}
454450

455-
//----------------------------------------------------------------------------
456-
static int op_clear(CPPInstance* pyobj)
457-
{
458-
// Garbage collector clear of held python member objects; this is a good time
459-
// to safely remove this object from the memory regulator.
460-
if (pyobj->fFlags & CPPInstance::kIsRegulated)
461-
MemoryRegulator::UnregisterPyObject(pyobj, (PyObject*)Py_TYPE((PyObject*)pyobj));
462-
463-
return 0;
451+
// Remove (Python-side) memory held by the object proxy.
452+
Py_TYPE(pyobj)->tp_free((PyObject*)pyobj);
464453
}
465454

466455
//----------------------------------------------------------------------------
@@ -1086,11 +1075,10 @@ PyTypeObject CPPInstance_Type = {
10861075
0, // tp_as_buffer
10871076
Py_TPFLAGS_DEFAULT |
10881077
Py_TPFLAGS_BASETYPE |
1089-
Py_TPFLAGS_CHECKTYPES |
1090-
Py_TPFLAGS_HAVE_GC, // tp_flags
1078+
Py_TPFLAGS_CHECKTYPES, // tp_flags
10911079
(char*)"cppyy object proxy (internal)", // tp_doc
10921080
(traverseproc)op_traverse, // tp_traverse
1093-
(inquiry)op_clear, // tp_clear
1081+
0, // tp_clear
10941082
(richcmpfunc)op_richcompare, // tp_richcompare
10951083
0, // tp_weaklistoffset
10961084
0, // tp_iter

0 commit comments

Comments
 (0)