Merge pull request #248 from Bluehorn/master

no_gc_clear implementation (disable tp_clear) + doc and tests

Cython/Compiler/ModuleNode.py

@@ -1009,6 +1009,7 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
                     self.generate_dealloc_function(scope, code)
                     if scope.needs_gc():
                         self.generate_traverse_function(scope, code, entry)
+                        if scope.needs_tp_clear():
                             self.generate_clear_function(scope, code, entry)
                     if scope.defines_any(["__getitem__"]):
                         self.generate_getitem_int_function(scope, code)
@@ -1367,7 +1368,6 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
 
         if py_attrs or py_buffers:
             self.generate_self_cast(scope, code)
-            code.putln("PyObject* tmp;")
 
         if base_type:
             # want to call it explicitly if possible so inlining can be performed
@@ -1390,13 +1390,7 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
                     UtilityCode.load_cached("CallNextTpClear", "ExtensionTypes.c"))
 
         for entry in py_attrs:
-            name = "p->%s" % entry.cname
-            code.putln("tmp = ((PyObject*)%s);" % name)
-            if entry.is_declared_generic:
-                code.put_init_to_py_none(name, py_object_type, nanny=False)
-            else:
-                code.put_init_to_py_none(name, entry.type, nanny=False)
-            code.putln("Py_XDECREF(tmp);")
+            code.putln("Py_CLEAR(p->%s);" % entry.cname)
 
         for entry in py_buffers:
             # Note: shouldn't this call __Pyx_ReleaseBuffer ??

Cython/Compiler/Options.py

@@ -96,6 +96,7 @@ directive_defaults = {
     'final' : False,
     'internal' : False,
     'profile': False,
+    'no_gc_clear': False,
     'linetrace': False,
     'infer_types': None,
     'infer_types.verbose': False,
@@ -214,6 +215,7 @@ for key, val in directive_defaults.items():
 directive_scopes = { # defaults to available everywhere
     # 'module', 'function', 'class', 'with statement'
     'final' : ('cclass', 'function'),
+    'no_gc_clear' : ('cclass',),
     'internal' : ('cclass',),
     'autotestdict' : ('module',),
     'autotestdict.all' : ('module',),

Cython/Compiler/Symtab.py

@@ -1804,6 +1804,13 @@ class CClassScope(ClassScope):
             return not self.parent_type.is_gc_simple
         return False
 
+    def needs_tp_clear(self):
+        """
+        Do we need to generate an implementation for the tp_clear slot? Can
+        be disabled to keep references for the __dealloc__ cleanup function.
+        """
+        return self.needs_gc() and not self.directives.get('no_gc_clear', False)
+
     def declare_var(self, name, type, pos,
                     cname = None, visibility = 'private',
                     api = 0, in_pxd = 0, is_cdef = 0):

Cython/Compiler/TypeSlots.py

@@ -331,6 +331,14 @@ class GCDependentSlot(InternalMethodSlot):
         return InternalMethodSlot.slot_code(self, scope)
 
 
+class GCClearReferencesSlot(GCDependentSlot):
+
+    def slot_code(self, scope):
+        if scope.needs_tp_clear():
+            return GCDependentSlot.slot_code(self, scope)
+        return "0"
+
+
 class ConstructorSlot(InternalMethodSlot):
     #  Descriptor for tp_new and tp_dealloc.
 
@@ -753,7 +761,7 @@ slot_table = (
     DocStringSlot("tp_doc"),
 
     GCDependentSlot("tp_traverse"),
-    GCDependentSlot("tp_clear"),
+    GCClearReferencesSlot("tp_clear"),
 
     # Later -- synthesize a method to split into separate ops?
     MethodSlot(richcmpfunc, "tp_richcompare", "__richcmp__", inherited=False),  # Py3 checks for __hash__

docs/src/userguide/extension_types.rst

@@ -469,6 +469,43 @@ object called :attr:`__weakref__`. For example,::
         cdef object __weakref__
 
 
+Controlling cyclic garbage collection in CPython
+================================================
+
+By default each extension type will support the cyclic garbage collector of
+CPython. If any Python objects can be referenced, Cython will automatically
+generate the ``tp_traverse`` and ``tp_clear`` slots. This is usually what you
+want.
+
+There is at least one reason why this might not be what you want: If you need
+to cleanup some external resources in the ``__dealloc__`` special function and
+your object happened to be in a reference cycle, the garbage collector may
+have triggered a call to ``tp_clear`` to drop references. This is the way that
+reference cycles are broken so that the garbage can actually be reclaimed.
+
+In that case any object references have vanished by the time when
+``__dealloc__`` is called. Now your cleanup code lost access to the objects it
+has to clean up. In that case you can disable the cycle breaker ``tp_clear``
+by using the ``no_gc_clear`` decorator ::
+
+    @cython.no_gc_clear
+    cdef class DBCursor:
+        cdef DBConnection conn
+        cdef DBAPI_Cursor *raw_cursor
+        # ...
+        def __dealloc__(self):
+            DBAPI_close_cursor(self.conn.raw_conn, self.raw_cursor)
+
+This example tries to close a cursor via a database connection when the Python
+object is destroyed. The ``DBConnection`` object is kept alive by the reference
+from ``DBCursor``. But if a cursor happens to be in a reference cycle, the
+garbage collector may effectively "steal" the database connection reference,
+which makes it impossible to clean up the cursor.
+
+Using the ``no_gc_clear`` decorator this can not happen anymore because the
+references of a cursor object will not be cleared anymore.
+
+
 Public and external extension types
 ====================================
 

tests/run/clear_to_null.pyx

+"""
+Check that Cython generates a tp_clear function that actually clears object
+references to NULL instead of None.
+
+Discussed here: http://article.gmane.org/gmane.comp.python.cython.devel/14833
+"""
+
+from cpython.ref cimport PyObject, Py_TYPE
+
+cdef class ExtensionType:
+    """
+    Just a type which is handled by a specific C type (instead of PyObject)
+    to check that tp_clear works when the C pointer is of a type different
+    from PyObject *.
+    """
+
+
+# Pull tp_clear for PyTypeObject as I did not find another way to access it
+# from Cython code.
+
+cdef extern from "Python.h":
+    ctypedef struct PyTypeObject:
+        void (*tp_clear)(object)
+
+
+cdef class TpClearFixture:
+    """
+    An extension type that has a tp_clear method generated to test that it
+    actually clears the references to NULL.
+
+    >>> fixture = TpClearFixture()
+    >>> isinstance(fixture.extension_type, ExtensionType)
+    True
+    >>> isinstance(fixture.any_object, str)
+    True
+    >>> fixture.call_tp_clear()
+    >>> fixture.check_any_object_status()
+    'NULL'
+    >>> fixture.check_extension_type_status()
+    'NULL'
+    """
+    
+    cdef readonly object any_object
+    cdef readonly ExtensionType extension_type
+
+    def __cinit__(self):
+        self.any_object = "Hello World"
+        self.extension_type = ExtensionType()
+
+    def call_tp_clear(self):
+        cdef PyTypeObject *pto = Py_TYPE(self)
+        pto.tp_clear(self)
+
+    def check_any_object_status(self):
+        if <void*>(self.any_object) == NULL:
+            return 'NULL'
+        elif self.any_object is None:
+            return 'None' 
+        else:
+            return 'not cleared'
+
+    def check_extension_type_status(self):
+        if <void*>(self.any_object) == NULL:
+            return 'NULL'
+        elif self.any_object is None:
+            return 'None' 
+        else:
+            return 'not cleared'

tests/run/no_gc_clear.pyx

+"""
+Check that the @cython.no_gc_clear decorator disables generation of the
+tp_clear slot so that __dealloc__ will still see the original reference
+contents.
+
+Discussed here: http://article.gmane.org/gmane.comp.python.cython.devel/14986
+"""
+
+cimport cython
+from cpython.ref cimport PyObject, Py_TYPE
+
+# Pull tp_clear for PyTypeObject as I did not find another way to access it
+# from Cython code.
+
+cdef extern from "Python.h":
+    ctypedef struct PyTypeObject:
+        void (*tp_clear)(object)
+
+
+@cython.no_gc_clear
+cdef class DisableTpClear:
+    """
+    An extension type that has a tp_clear method generated to test that it
+    actually clears the references to NULL.
+
+    >>> uut = DisableTpClear()
+    >>> uut.call_tp_clear()
+    >>> type(uut.requires_cleanup) == list
+    True
+    >>> del uut
+    """
+
+    cdef public object requires_cleanup
+
+    def __cinit__(self):
+        self.requires_cleanup = [
+                "Some object that needs cleaning in __dealloc__"]
+
+    cpdef public call_tp_clear(self):
+        cdef PyTypeObject *pto = Py_TYPE(self)
+        if pto.tp_clear != NULL:
+            pto.tp_clear(self)