Skip to content

Commit f9c614b

Browse files
committed
[Python] Don't exploit garbage collection hooks for memory regulator
Looking at the CPPInstance, I noticed that is claims to support cycling garbage collection via the `Py_TPFLAGS_HAVE_GC`, but it doesn't stick completely to the contract explained in the docs: https://docs.python.org/3/c-api/gcsupport.html#c.PyObject_GC_Track In particular, it breaks the constructor rules: * The memory for the object must be allocated using PyObject_GC_New or PyObject_GC_NewVar. * Once all the fields which may contain references to other containers are initialized, it must call PyObject_GC_Track(). The `tp_traverse` and `tp_clear` implementations actually do nothing with the C Python API, so from the point of view of Python, there is no reason to support cyclic garbage collection. I think the CPPInstance tries to exploit the Python garbage collector to trigger its own memory regulator code path early in `tp_clear`, but breaking some of the Python C API contracts on the way. And the C++ object is also unregisterd from the memory regulator in `tp_dealloc` anyway. 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. This reverts parts of the following CPyCppy commits from 2019: * wlav/CPyCppyy@e43759a * wlav/CPyCppyy@29eb42e
1 parent 3ab1d5c commit f9c614b

File tree

1 file changed

+3
-16
lines changed

1 file changed

+3
-16
lines changed

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

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -447,20 +447,8 @@ static CPPInstance* op_new(PyTypeObject* subtype, PyObject*, PyObject*)
447447
static void op_dealloc(CPPInstance* pyobj)
448448
{
449449
// Remove (Python-side) memory held by the object proxy.
450-
PyObject_GC_UnTrack((PyObject*)pyobj);
451450
op_dealloc_nofree(pyobj);
452-
PyObject_GC_Del((PyObject*)pyobj);
453-
}
454-
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+
Py_TYPE(pyobj)->tp_free((PyObject*)pyobj);
464452
}
465453

466454
//----------------------------------------------------------------------------
@@ -1086,11 +1074,10 @@ PyTypeObject CPPInstance_Type = {
10861074
0, // tp_as_buffer
10871075
Py_TPFLAGS_DEFAULT |
10881076
Py_TPFLAGS_BASETYPE |
1089-
Py_TPFLAGS_CHECKTYPES |
1090-
Py_TPFLAGS_HAVE_GC, // tp_flags
1077+
Py_TPFLAGS_CHECKTYPES, // tp_flags
10911078
(char*)"cppyy object proxy (internal)", // tp_doc
10921079
(traverseproc)op_traverse, // tp_traverse
1093-
(inquiry)op_clear, // tp_clear
1080+
0, // tp_clear
10941081
(richcmpfunc)op_richcompare, // tp_richcompare
10951082
0, // tp_weaklistoffset
10961083
0, // tp_iter

0 commit comments

Comments
 (0)