diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index 7b35ba60b53a75..4d6e2f21551a6c 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -383,6 +383,92 @@ def __del__(self): del L self.assertEqual(PyList.num, 0) + def test_subclass_of_heap_gc_ctype_with_tpdealloc_decrefs_once(self): + class HeapGcCTypeSubclass(_testcapi.HeapGcCType): + def __init__(self): + self.value2 = 20 + super().__init__() + + subclass_instance = HeapGcCTypeSubclass() + type_refcnt = sys.getrefcount(HeapGcCTypeSubclass) + + # Test that subclass instance was fully created + self.assertEqual(subclass_instance.value, 10) + self.assertEqual(subclass_instance.value2, 20) + + # Test that the type reference count is only decremented once + del subclass_instance + self.assertEqual(type_refcnt - 1, sys.getrefcount(HeapGcCTypeSubclass)) + + def test_subclass_of_heap_gc_ctype_with_del_modifying_dunder_class_only_decrefs_once(self): + class A(_testcapi.HeapGcCType): + def __init__(self): + self.value2 = 20 + super().__init__() + + class B(A): + def __init__(self): + super().__init__() + + def __del__(self): + self.__class__ = A + A.refcnt_in_del = sys.getrefcount(A) + B.refcnt_in_del = sys.getrefcount(B) + + subclass_instance = B() + type_refcnt = sys.getrefcount(B) + new_type_refcnt = sys.getrefcount(A) + + # Test that subclass instance was fully created + self.assertEqual(subclass_instance.value, 10) + self.assertEqual(subclass_instance.value2, 20) + + del subclass_instance + + # Test that setting __class__ modified the reference counts of the types + self.assertEqual(type_refcnt - 1, B.refcnt_in_del) + self.assertEqual(new_type_refcnt + 1, A.refcnt_in_del) + + # Test that the original type already has decreased its refcnt + self.assertEqual(type_refcnt - 1, sys.getrefcount(B)) + + # Test that subtype_dealloc decref the newly assigned __class__ only once + self.assertEqual(new_type_refcnt, sys.getrefcount(A)) + + def test_c_subclass_of_heap_ctype_with_tpdealloc_decrefs_once(self): + subclass_instance = _testcapi.HeapCTypeSubclass() + type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclass) + + # Test that subclass instance was fully created + self.assertEqual(subclass_instance.value, 10) + self.assertEqual(subclass_instance.value2, 20) + + # Test that the type reference count is only decremented once + del subclass_instance + self.assertEqual(type_refcnt - 1, sys.getrefcount(_testcapi.HeapCTypeSubclass)) + + def test_c_subclass_of_heap_ctype_with_del_modifying_dunder_class_only_decrefs_once(self): + subclass_instance = _testcapi.HeapCTypeSubclassWithFinalizer() + type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclassWithFinalizer) + new_type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclass) + + # Test that subclass instance was fully created + self.assertEqual(subclass_instance.value, 10) + self.assertEqual(subclass_instance.value2, 20) + + # The tp_finalize slot will set __class__ to HeapCTypeSubclass + del subclass_instance + + # Test that setting __class__ modified the reference counts of the types + self.assertEqual(type_refcnt - 1, _testcapi.HeapCTypeSubclassWithFinalizer.refcnt_in_del) + self.assertEqual(new_type_refcnt + 1, _testcapi.HeapCTypeSubclass.refcnt_in_del) + + # Test that the original type already has decreased its refcnt + self.assertEqual(type_refcnt - 1, sys.getrefcount(_testcapi.HeapCTypeSubclassWithFinalizer)) + + # Test that subtype_dealloc decref the newly assigned __class__ only once + self.assertEqual(new_type_refcnt, sys.getrefcount(_testcapi.HeapCTypeSubclass)) + class TestPendingCalls(unittest.TestCase): diff --git a/Misc/NEWS.d/next/C API/2019-08-17-13-50-21.bpo-37879.CZeUem.rst b/Misc/NEWS.d/next/C API/2019-08-17-13-50-21.bpo-37879.CZeUem.rst new file mode 100644 index 00000000000000..87322fbf5f239b --- /dev/null +++ b/Misc/NEWS.d/next/C API/2019-08-17-13-50-21.bpo-37879.CZeUem.rst @@ -0,0 +1,2 @@ +Fix subtype_dealloc to suppress the type decref when the base type is a C +heap type diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 2b261b6c86dc4b..21061a2171a052 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -32,6 +32,8 @@ # error "_testcapi must test the public Python C API, not CPython internal C API" #endif +static struct PyModuleDef _testcapimodule; + static PyObject *TestError; /* set to exception object in init */ /* Raise TestError with test_name + ": " + msg, and return NULL. */ @@ -6169,6 +6171,189 @@ static PyTypeObject MethodDescriptor2_Type = { .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | _Py_TPFLAGS_HAVE_VECTORCALL, }; +PyDoc_STRVAR(heapgctype__doc__, +"A heap type with GC, and with overridden dealloc.\n\n" +"The 'value' attribute is set to 10 in __init__."); + +typedef struct { + PyObject_HEAD + int value; +} HeapCTypeObject; + +static struct PyMemberDef heapctype_members[] = { + {"value", T_INT, offsetof(HeapCTypeObject, value)}, + {NULL} /* Sentinel */ +}; + +static int +heapctype_init(PyObject *self, PyObject *args, PyObject *kwargs) +{ + ((HeapCTypeObject *)self)->value = 10; + return 0; +} + +static void +heapgcctype_dealloc(HeapCTypeObject *self) +{ + PyTypeObject *tp = Py_TYPE(self); + PyObject_GC_UnTrack(self); + PyObject_GC_Del(self); + Py_DECREF(tp); +} + +static PyType_Slot HeapGcCType_slots[] = { + {Py_tp_init, heapctype_init}, + {Py_tp_members, heapctype_members}, + {Py_tp_dealloc, heapgcctype_dealloc}, + {Py_tp_doc, heapgctype__doc__}, + {0, 0}, +}; + +static PyType_Spec HeapGcCType_spec = { + "_testcapi.HeapGcCType", + sizeof(HeapCTypeObject), + 0, + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HAVE_GC, + HeapGcCType_slots +}; + +PyDoc_STRVAR(heapctype__doc__, +"A heap type without GC, but with overridden dealloc.\n\n" +"The 'value' attribute is set to 10 in __init__."); + +static void +heapctype_dealloc(HeapCTypeObject *self) +{ + PyTypeObject *tp = Py_TYPE(self); + PyObject_Del(self); + Py_DECREF(tp); +} + +static PyType_Slot HeapCType_slots[] = { + {Py_tp_init, heapctype_init}, + {Py_tp_members, heapctype_members}, + {Py_tp_dealloc, heapctype_dealloc}, + {Py_tp_doc, heapctype__doc__}, + {0, 0}, +}; + +static PyType_Spec HeapCType_spec = { + "_testcapi.HeapCType", + sizeof(HeapCTypeObject), + 0, + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, + HeapCType_slots +}; + +PyDoc_STRVAR(heapctypesubclass__doc__, +"Subclass of HeapCType, without GC.\n\n" +"__init__ sets the 'value' attribute to 10 and 'value2' to 20."); + +typedef struct { + HeapCTypeObject base; + int value2; +} HeapCTypeSubclassObject; + +static int +heapctypesubclass_init(PyObject *self, PyObject *args, PyObject *kwargs) +{ + /* Call __init__ of the superclass */ + if (heapctype_init(self, args, kwargs) < 0) { + return -1; + } + /* Initialize additional element */ + ((HeapCTypeSubclassObject *)self)->value2 = 20; + return 0; +} + +static struct PyMemberDef heapctypesubclass_members[] = { + {"value2", T_INT, offsetof(HeapCTypeSubclassObject, value2)}, + {NULL} /* Sentinel */ +}; + +static PyType_Slot HeapCTypeSubclass_slots[] = { + {Py_tp_init, heapctypesubclass_init}, + {Py_tp_members, heapctypesubclass_members}, + {Py_tp_doc, heapctypesubclass__doc__}, + {0, 0}, +}; + +static PyType_Spec HeapCTypeSubclass_spec = { + "_testcapi.HeapCTypeSubclass", + sizeof(HeapCTypeSubclassObject), + 0, + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, + HeapCTypeSubclass_slots +}; + +PyDoc_STRVAR(heapctypesubclasswithfinalizer__doc__, +"Subclass of HeapCType with a finalizer that reassigns __class__.\n\n" +"__class__ is set to plain HeapCTypeSubclass during finalization.\n" +"__init__ sets the 'value' attribute to 10 and 'value2' to 20."); + +static int +heapctypesubclasswithfinalizer_init(PyObject *self, PyObject *args, PyObject *kwargs) +{ + PyTypeObject *base = (PyTypeObject *)PyType_GetSlot(Py_TYPE(self), Py_tp_base); + initproc base_init = PyType_GetSlot(base, Py_tp_init); + base_init(self, args, kwargs); + return 0; +} + +static void +heapctypesubclasswithfinalizer_finalize(PyObject *self) +{ + PyObject *error_type, *error_value, *error_traceback, *m, *oldtype, *newtype; + + /* Save the current exception, if any. */ + PyErr_Fetch(&error_type, &error_value, &error_traceback); + + m = PyState_FindModule(&_testcapimodule); + if (m == NULL) { + goto cleanup_finalize; + } + oldtype = PyObject_GetAttrString(m, "HeapCTypeSubclassWithFinalizer"); + newtype = PyObject_GetAttrString(m, "HeapCTypeSubclass"); + if (oldtype == NULL || newtype == NULL) { + goto cleanup_finalize; + } + + if (PyObject_SetAttrString(self, "__class__", newtype) < 0) { + goto cleanup_finalize; + } + if (PyObject_SetAttrString( + oldtype, "refcnt_in_del", PyLong_FromSsize_t(Py_REFCNT(oldtype))) < 0) { + goto cleanup_finalize; + } + if (PyObject_SetAttrString( + newtype, "refcnt_in_del", PyLong_FromSsize_t(Py_REFCNT(newtype))) < 0) { + goto cleanup_finalize; + } + +cleanup_finalize: + Py_XDECREF(oldtype); + Py_XDECREF(newtype); + + /* Restore the saved exception. */ + PyErr_Restore(error_type, error_value, error_traceback); +} + +static PyType_Slot HeapCTypeSubclassWithFinalizer_slots[] = { + {Py_tp_init, heapctypesubclasswithfinalizer_init}, + {Py_tp_members, heapctypesubclass_members}, + {Py_tp_finalize, heapctypesubclasswithfinalizer_finalize}, + {Py_tp_doc, heapctypesubclasswithfinalizer__doc__}, + {0, 0}, +}; + +static PyType_Spec HeapCTypeSubclassWithFinalizer_spec = { + "_testcapi.HeapCTypeSubclassWithFinalizer", + sizeof(HeapCTypeSubclassObject), + 0, + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_FINALIZE, + HeapCTypeSubclassWithFinalizer_slots +}; + static PyMethodDef meth_instance_methods[] = { {"meth_varargs", meth_varargs, METH_VARARGS}, {"meth_varargs_keywords", (PyCFunction)(void(*)(void))meth_varargs_keywords, METH_VARARGS|METH_KEYWORDS}, @@ -6380,5 +6565,40 @@ PyInit__testcapi(void) TestError = PyErr_NewException("_testcapi.error", NULL, NULL); Py_INCREF(TestError); PyModule_AddObject(m, "error", TestError); + + PyObject *HeapGcCType = PyType_FromSpec(&HeapGcCType_spec); + if (HeapGcCType == NULL) { + return NULL; + } + PyModule_AddObject(m, "HeapGcCType", HeapGcCType); + + PyObject *HeapCType = PyType_FromSpec(&HeapCType_spec); + if (HeapCType == NULL) { + return NULL; + } + PyObject *subclass_bases = PyTuple_Pack(1, HeapCType); + if (subclass_bases == NULL) { + return NULL; + } + PyObject *HeapCTypeSubclass = PyType_FromSpecWithBases(&HeapCTypeSubclass_spec, subclass_bases); + if (HeapCTypeSubclass == NULL) { + return NULL; + } + Py_DECREF(subclass_bases); + PyModule_AddObject(m, "HeapCTypeSubclass", HeapCTypeSubclass); + + PyObject *subclass_with_finalizer_bases = PyTuple_Pack(1, HeapCTypeSubclass); + if (subclass_with_finalizer_bases == NULL) { + return NULL; + } + PyObject *HeapCTypeSubclassWithFinalizer = PyType_FromSpecWithBases( + &HeapCTypeSubclassWithFinalizer_spec, subclass_with_finalizer_bases); + if (HeapCTypeSubclassWithFinalizer == NULL) { + return NULL; + } + Py_DECREF(subclass_with_finalizer_bases); + PyModule_AddObject(m, "HeapCTypeSubclassWithFinalizer", HeapCTypeSubclassWithFinalizer); + + PyState_AddModule(m, &_testcapimodule); return m; } diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 883bc22bbde1be..7575e5580bf98c 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -1157,11 +1157,9 @@ subtype_dealloc(PyObject *self) /* Test whether the type has GC exactly once */ if (!PyType_IS_GC(type)) { - /* It's really rare to find a dynamic type that doesn't have - GC; it can only happen when deriving from 'object' and not - adding any slots or instance variables. This allows - certain simplifications: there's no need to call - clear_slots(), or DECREF the dict, or clear weakrefs. */ + /* A non GC dynamic type allows certain simplifications: + there's no need to call clear_slots(), or DECREF the dict, + or clear weakrefs. */ /* Maybe call finalizer; exit early if resurrected */ if (type->tp_finalize) { @@ -1177,7 +1175,6 @@ subtype_dealloc(PyObject *self) /* Find the nearest base with a different tp_dealloc */ base = type; while ((basedealloc = base->tp_dealloc) == subtype_dealloc) { - assert(Py_SIZE(base) == 0); base = base->tp_base; assert(base); } @@ -1189,8 +1186,10 @@ subtype_dealloc(PyObject *self) assert(basedealloc); basedealloc(self); - /* Can't reference self beyond this point */ - Py_DECREF(type); + /* Only decref if the base type is not already a heap allocated type. + Otherwise, basedealloc should have decref'd it already */ + if (type->tp_flags & Py_TPFLAGS_HEAPTYPE && !(base->tp_flags & Py_TPFLAGS_HEAPTYPE)) + Py_DECREF(type); /* Done */ return; @@ -1289,8 +1288,9 @@ subtype_dealloc(PyObject *self) /* Can't reference self beyond this point. It's possible tp_del switched our type from a HEAPTYPE to a non-HEAPTYPE, so be careful about - reference counting. */ - if (type->tp_flags & Py_TPFLAGS_HEAPTYPE) + reference counting. Only decref if the base type is not already a heap + allocated type. Otherwise, basedealloc should have decref'd it already */ + if (type->tp_flags & Py_TPFLAGS_HEAPTYPE && !(base->tp_flags & Py_TPFLAGS_HEAPTYPE)) Py_DECREF(type); endlabel: