ZODB Components: Modules loaded by one Request were being GC'ed while executing in another Request.

Until this commit, loading a ZODB Component would add it to the current Request
object to prevent its reference counter reaching 0 (thus its global being reset to
None and then being later GC'ed) when a reset happens in another Request.

This problem was found when investigating testFunctionalConfigurator failure
when installing bt5s as requested by the Configurator:
  1. Request R1 calls Zuite_waitForActivities importing extension.erp5.ERP5Zuite (M1).
     => Add M1 to R1._module_cache_set.
     => M1 ref counter equals to 2 (sys.modules and R1._module_cache_set references).
  2. R1 terminates and is GC'ed.
     => M1 ref counter equals to 1 (sys.modules).
  3. Request R2 runs Configurator configuring the Site.
  4. testFunctionalConfigurator calls Zuite_waitForActivities to wait for the
     Configurator to finish (request R3 which may take ~15 minutes). This calls
     time.sleep() in a loop where 'time' module is imported at top-level.
  5. R2 installs bt5 triggering reset.
     => M1 ref counter equals to 0.
        ===> M1 global variables are reset to None and thus 'time' is set to None
             raising an Exception in the next call of time.sleep() in the loop.

The easiest way would be to have a hook on sys.modules dict lookup and thus add M1
to R2._module_cache_set when being imported, but this is not supported... Instead
create a global cache on erp5.component package.

product/ERP5/tests/testBusinessTemplate.py

@@ -7485,7 +7485,7 @@ class TestBusinessTemplate(BusinessTemplateMixin):
 
   def stepSimulateToCreateNewRequest(self, sequence=None, **kw):
     """
-     Remove the caches in _module_cache_set to simulate to create new REQUEST
+     Clear the Request ZODB Component module caches to simulate a new REQUEST.
 
      Why we need this is because:
      - ZODB Components relies on this cache to prevend gc of the component,
@@ -7494,11 +7494,8 @@ class TestBusinessTemplate(BusinessTemplateMixin):
        even if the request (transaction) is finished.
      This removal imitates the behavior in a real ZOPE environment
     """
-    from Products.ERP5Type.Globals import get_request
-    request_obj = get_request()
-    module_cache_set = getattr(request_obj, '_module_cache_set', None)
-    # delete the reference (decrement the reference count)
-    module_cache_set.clear()
+    import erp5.component
+    erp5.component.request_cache_clear()
 
   def stepAddExtensionComponentToBusinessTemplate(self, sequence=None, **kw):
     sequence['current_bt'].setProperty('template_extension_id_list',

product/ERP5Type/Tool/ComponentTool.py

@@ -151,6 +151,7 @@ class ComponentTool(BaseTool):
         if isinstance(package, ComponentDynamicPackage):
           package.reset()
 
+      erp5.component.request_cache_clean()
     if reset_portal_type_at_transaction_boundary:
       portal.portal_types.resetDynamicDocumentsOnceAtTransactionBoundary()
     else:

product/ERP5Type/ZopePatch.py

@@ -21,6 +21,7 @@
 ##############################################################################
 
 # Load all monkey patches
+from Products.ERP5Type.patches import HTTPRequest
 from Products.ERP5Type.patches import Restricted
 from Products.ERP5Type.patches import m2crypto
 from Products.ERP5Type.patches import ObjectManager

product/ERP5Type/dynamic/component_package.py

@@ -35,7 +35,6 @@ import imp
 import collections
 
 from Products.ERP5.ERP5Site import getSite
-from Products.ERP5Type.Globals import get_request
 from . import aq_method_lock
 from types import ModuleType
 from zLOG import LOG, BLATHER, WARNING
@@ -304,22 +303,8 @@ class ComponentDynamicPackage(ModuleType):
       if module_fullname_alias:
         setattr(self, name, module)
 
-      # When the reference counter of a module reaches 0, its globals are all
-      # reset to None. So, if a thread performs a reset while another one
-      # executes codes using globals (such as modules imported at module level),
-      # the latter one must keep a reference around to avoid reaching a
-      # reference count to 0. Thus, add it to Request object.
-      #
-      # OTOH, this means that ZODB Components module *must* be imported at the
-      # top level, otherwise a module being relied upon may have a different API
-      # after rset, thus it may fail...
-      request_obj = get_request()
-      module_cache_set = getattr(request_obj, '_module_cache_set', None)
-      if module_cache_set is None:
-        module_cache_set = set()
-        request_obj._module_cache_set = module_cache_set
-
-      module_cache_set.add(module)
+      import erp5.component
+      erp5.component.request_cache_add_module(module)
 
       return module
     finally:

product/ERP5Type/dynamic/dynamic_module.py

@@ -39,6 +39,87 @@ class PackageType(ModuleType):
   """
   __path__ = []
 
+class ComponentPackageType(PackageType):
+  """
+  Package for ZODB Component keeping reference to ZODB Component module.
+
+  When the reference counter of a module reaches 0, its globals are
+  all reset to None. So, if a thread performs a reset while another
+  one executes codes using globals (such as modules imported at module
+  level), the latter one must keep a reference around to avoid
+  reaching a reference count to 0.
+
+  Initially, this reference was kept in the Request object itself, but
+  this does not work with the following use case:
+
+    1. R1 loads M1 and keep a reference in R1.
+    2. R2 imports M1.
+       => Hit sys.modules and not going through Import Hooks.
+          => No way to know that this module is being used by R2.
+    3. R1 finishes and is destroyed.
+       => M1 reference counter reaches 0 => globals set to None.
+    4. R2 uses a global in M1.
+
+  Thus create a cache per 'last_sync' and keep a module reference for
+  *all* last_sync in case a module is imported by another earlier
+  last_sync Request.
+
+  OTOH, this means that ZODB Components module *must* be imported at
+  the top level, otherwise a module being relied upon may have a
+  different API after reset, thus it may fail...
+  """
+  def __init__(self, *args, **kwargs):
+    super(ComponentPackageType, self).__init__(*args, **kwargs)
+    self._request_cache_dict = {}
+
+  def request_cache_add(self, request_obj):
+    from Products.ERP5Type.Tool.ComponentTool import last_sync
+    from weakref import WeakSet
+    # dict.setdefault() atomic from 2.7.3
+    self._request_cache_dict.setdefault(
+      last_sync, (WeakSet(), set()))[0].add(request_obj)
+
+  def request_cache_add_module(self, module_obj):
+    # Zope Ready to handle requests
+    #
+    # * R1:
+    # -> last_sync=-1: HTTPRequest.__init__:
+    #    request_module_dict: {-1: ([R1], [])}
+    # ERP5Type.Tool.ComponentTool Resetting Components
+    # -> last_sync=12: ComponentTool.reset:
+    #    request_module_dict: {-1: ([R1], [])}
+    # -> last_sync=12: C1.__load_module:
+    #    request_module_dict: {-1: ([R1], [C1])}
+    # => R1 finished and can be gc'ed.
+    #
+    # * R2 using C1:
+    # -> last_sync=12:
+    #    request_module_dict: {-1: ([], [C1])}
+    #
+    # => While R2 is running, a reset is performed and clear '-1'
+    #    => C1 is gc'ed.
+    from Products.ERP5Type.Globals import get_request
+    self.request_cache_add(get_request())
+
+    for (_, module_obj_set) in self._request_cache_dict.itervalues():
+      module_obj_set.add(module_obj)
+
+  def request_cache_clean(self):
+    """
+    Remove cache items with no Request Left.
+    """
+    for (current_last_sync,
+         (request_obj_weakset, _)) in self._request_cache_dict.items():
+      if not request_obj_weakset:
+        del self._request_cache_dict[current_last_sync]
+
+  def request_cache_clear(self):
+    """
+    Completely clear the cache. Should never be called except to
+    simulate new Requests in Unit Tests for example.
+    """
+    self._request_cache_dict.clear()
+
 class DynamicModule(ModuleType):
   """This module may generate new objects at runtime."""
   # it's useful to have such a generic utility
@@ -130,7 +211,7 @@ def initializeDynamicModules():
                                                 loadTempPortalTypeClass)
 
   # ZODB Components
-  erp5.component = PackageType("erp5.component")
+  erp5.component = ComponentPackageType("erp5.component")
 
   from component_package import ComponentDynamicPackage
 

product/ERP5Type/patches/HTTPRequest.py

+# -*- coding: utf-8 -*-
+
+from ZPublisher.HTTPRequest import HTTPRequest
+import sys
+import weakref
+import thread
+
+HTTPRequest__init__ = HTTPRequest.__init__
+def __init__(self, *args, **kw):
+  HTTPRequest__init__(self, *args, **kw)
+
+  import erp5.component
+  erp5.component.request_cache_add(self)
+
+HTTPRequest.__init__ = __init__

product/ERP5Type/tests/ERP5TypeLiveTestCase.py

@@ -83,12 +83,6 @@ class ERP5TypeLiveTestCase(ERP5TypeTestCaseMixin):
       if self.portal is not None:
         return self.portal
 
-      # _module_cache_set is used to keep a reference to the code of modules
-      # before they get reloaded. As we will use another request we need to
-      # make sure that we still have a reference to _module_cache_set so that
-      # it does not get garbage collected.
-      module_cache_set = getattr(get_request(), '_module_cache_set', None)
-
       from Products.ERP5.ERP5Site import getSite
       site = getSite()
       # reconstruct the acquistion chain with an independant request.
@@ -96,9 +90,6 @@ class ERP5TypeLiveTestCase(ERP5TypeTestCaseMixin):
       from Testing.ZopeTestCase.utils import makerequest
       portal = getattr(makerequest(site.aq_parent), site.getId())
 
-      if module_cache_set:
-        portal.REQUEST._module_cache_set = module_cache_set
-
       # Make the various get_request patches return this request.
       # This is for ERP5TypeTestCase patch
       from Testing.ZopeTestCase.connections import registry