ZODB Components: Resetting Portal Type classes was also clearing Interfaces of...

ZODB Components: Resetting Portal Type classes was also clearing Interfaces of erp5.component.* modules.

__implemented__ and __provides__ attributes are cleared on Portal Type reset
to break a circular reference (11e2b506). Resetting of ZODB Components is
done independently of Portal Type classes, so until the next ZODB Component
reset, Interfaces were not implemented anymore.

11e2b506 states that this creates a memory leak but this is not actually true
as Python garbage collector will break such circular reference and collect
this object, as asserted by a newly introduced Unit Test. This is probably
less efficient than explicitly breaking the circular reference and thus relies
on reference counting (rather than the garbage collector) but doing otherwise
would require introspecting ZODB Components module to find classes implementing
Interfaces (erp5.portal_type only contains classes).

This only takes care of ZODB Components modules but maybe breaking circular
references for Portal Type classes is not even needed (if so, this will be done
in a later commit).

product/ERP5Type/dynamic/portal_type_class.py

@@ -476,19 +476,20 @@ def synchronizeDynamicModules(context, force=False):
 
     LOG("ERP5Type.dynamic", 0, "Resetting dynamic classes")
     try:
-      for class_name, klass in inspect.getmembers(erp5.portal_type,
-                                                  inspect.isclass):
+      for _, klass in inspect.getmembers(erp5.portal_type,
+                                         inspect.isclass):
         # Zope Interface is implemented through __implements__,
         # __implemented__ (both implementedBy instances) and __provides__
         # (ClassProvides instance) attributes set on the class by
         # zope.interface.declarations.implementedByFallback.
         #
         # However both implementedBy and ClassProvides instances keep a
-        # reference to the class itself, thus creating a circular references
-        # preventing erp5.* classes to be GC even when not being actually used
-        # anywhere anymore after a reset.
+        # reference to the class itself, thus creating a circular references.
         for k in klass.mro():
-          if k.__module__.startswith('erp5.'):
+          module_name = k.__module__
+          if (module_name.startswith('erp5.') and
+              # Components are reset independently of Portal Types classes
+              not module_name.startswith('erp5.component.')):
             for attr in ('__implements__', '__implemented__', '__provides__'):
               if k.__dict__.get(attr) is not None:
                 delattr(k, attr)

product/ERP5Type/tests/testDynamicClassGeneration.py

@@ -2599,6 +2599,20 @@ class DifferentFromReference(Person):
     that the newly-defined function on ZODB Component can be called as well as
     methods from Person Document
     """
+    ## Create an Interface assigned to the test ZODB Component to check that
+    ## only resetting Portal Type classes do not have any side-effect on
+    ## Interfaces defined on ZODB Components
+    from zope.interface import Interface
+    class ITestPortalType(Interface):
+      """Anything"""
+      def foo():
+        """Anything"""
+    from types import ModuleType
+    interface_module = ModuleType('ITestPortalType')
+    interface_module.ITestPortalType = ITestPortalType
+    import sys
+    sys.modules['ITestPortalType'] = interface_module
+
     self.failIfModuleImportable('TestPortalType')
 
     # Create a new Document Component inheriting from Person Document which
@@ -2610,17 +2624,27 @@ class DifferentFromReference(Person):
       """
 from erp5.component.document.Person import Person
 
+from ITestPortalType import ITestPortalType
+import zope.interface
+
 class TestPortalType(Person):
   def test42(self):
     return 42
-""")
 
+  zope.interface.implements(ITestPortalType)
+  def foo(self):
+    pass
+""")
     test_component.validate()
     self.tic()
 
     # As TestPortalType Document Component has been validated, it should now
     # be available
-    self.assertModuleImportable('TestPortalType')
+    self.assertModuleImportable('TestPortalType', reset=False)
+    self.assertTrue(ITestPortalType.implementedBy(self._module.TestPortalType.TestPortalType))
+
+    self._component_tool.reset(force=True,
+                               reset_portal_type_at_transaction_boundary=True)
 
     person_type = self.portal.portal_types.Person
     person_type_class = person_type.getTypeClass()
@@ -2636,19 +2660,12 @@ class TestPortalType(Person):
     # assigned to a Person
     self.failIfHasAttribute(person, 'test42')
     self.failIfHasAttribute(self._module, 'TestPortalType')
+    self.assertFalse(ITestPortalType.providedBy(person))
+    self.assertFalse(ITestPortalType.implementedBy(person.__class__))
     for klass in person.__class__.mro():
       self.assertNotEqual(klass.__name__, 'TestPortalType')
 
-    # Reset Portal Type classes to ghost to make sure that everything is reset
-    self._component_tool.reset(force=True,
-                               reset_portal_type_at_transaction_boundary=True)
-
-    # TestPortalType must be available in type class list
-    self.assertTrue('TestPortalType' in person_type.getDocumentTypeList())
-    try:
-      person_type.setTypeClass('TestPortalType')
-      self.commit()
-
+    def _check():
       self.assertHasAttribute(person, 'test42')
       self.assertEqual(person.test42(), 42)
 
@@ -2658,6 +2675,21 @@ class TestPortalType(Person):
       self.assertTrue(self._module.TestPortalType.TestPortalType in person.__class__.mro())
       from erp5.component.document.Person import Person as PersonDocument
       self.assertTrue(PersonDocument in person.__class__.mro())
+      self.assertTrue(ITestPortalType.providedBy(person))
+      self.assertTrue(ITestPortalType.implementedBy(person.__class__))
+
+    # Reset Portal Type classes to ghost to make sure that everything is reset
+    self._component_tool.reset(force=True,
+                               reset_portal_type_at_transaction_boundary=False)
+    # TestPortalType must be available in type class list
+    self.assertTrue('TestPortalType' in person_type.getDocumentTypeList())
+    try:
+      person_type.setTypeClass('TestPortalType')
+      self.commit()
+      _check()
+
+      self.portal.portal_types.resetDynamicDocuments()
+      _check()
 
     finally:
       person_type.setTypeClass('Person')
@@ -2693,6 +2725,101 @@ class TestWithImport(TestImported):
     self.assertModuleImportable('TestWithImport')
     self.assertModuleImportable('TestImported')
 
+  def testGC(self):
+    """
+    Zope Implements and ClassProvides keep a reference to the class itself,
+    thus creating a circular reference which can only be garbage collected by
+    'gc' module (and not by reference counting).
+
+    So check that ZODB Components modules are properly garbage collectable
+    after a reset (in 'gc' terms: considered 'unreachable' but 'collectable'
+    and could be freed).
+    """
+    from zope.interface import Interface
+    class ITestGC(Interface):
+      """Anything"""
+      def foo():
+        """Anything"""
+    from types import ModuleType
+    interface_module = ModuleType('ITestGC')
+    interface_module.ITestGC = ITestGC
+    import sys
+    sys.modules['ITestGC'] = interface_module
+
+    self.failIfModuleImportable('TestGC')
+    test_component = self._newComponent(
+      'TestGC',
+      """from Products.ERP5Type.XMLObject import XMLObject
+from ITestGC import ITestGC
+import zope.interface
+
+class TestGC(XMLObject):
+  zope.interface.implements(ITestGC)
+  def foo(self):
+      pass
+""")
+    self.tic()
+    self.failIfModuleImportable('TestGC')
+
+    test_component.validate()
+    self.tic()
+
+    import gc
+    initial_gc_debug_flags = gc.get_debug()
+    initial_stderr = sys.stderr
+    try:
+      gc.disable()
+
+      gc.collect()
+      self.assertModuleImportable('TestGC', reset=False)
+      class_id = id(self._module.TestGC.TestGC)
+      Implements_id = id(self._module.TestGC.TestGC.__implemented__)
+      ClassProvides_id = id(self._module.TestGC.TestGC.__provides__)
+      self.assertEqual(gc.collect(), 0)
+      self.assertEqual(gc.garbage, [])
+
+      self._component_tool.reset(force=True)
+      gc.collect()
+      self.assertEqual(gc.garbage, [])
+
+      import sys
+      from cStringIO import StringIO
+      import erp5.component
+      gc.set_debug(
+        gc.DEBUG_STATS |
+        gc.DEBUG_UNCOLLECTABLE |
+        gc.DEBUG_COLLECTABLE |
+        gc.DEBUG_OBJECTS |
+        gc.DEBUG_INSTANCES)
+      stderr = StringIO()
+      sys.stderr = stderr
+      # Still not garbage collectable as RefManager still keeps a reference
+      erp5.component.ref_manager.clear()
+      # Once deleted, the ZODB Component module must be collectable...
+      self.assertNotEqual(gc.collect(), 0)
+
+    finally:
+      gc.set_debug(initial_gc_debug_flags)
+      gc.enable()
+
+      sys.stderr = initial_stderr
+
+    # And make sure that it has really be collected thanks to DEBUG_COLLECTABLE
+    self.assertEqual(gc.garbage, [])
+    stderr.seek(0)
+    found_line_list = []
+    for line in stderr:
+      if ('0x%x>' % class_id in line or
+          '0x%x>' % Implements_id in line or
+          '0x%x>' % ClassProvides_id in line):
+        found_line_list.append(line)
+
+    self.assertEqual(
+      ['gc: collectable <ClassProvides 0x%x>\n' % ClassProvides_id,
+       'gc: collectable <ExtensionClass.ExtensionClass 0x%x>\n' % class_id,
+       'gc: collectable <Implements 0x%x>\n' % Implements_id],
+      sorted(found_line_list))
+
 from Products.ERP5Type.Core.TestComponent import TestComponent
 
 class TestZodbTestComponent(_TestZodbComponent):