Commit f523edc4 authored by Arnaud Fontaine's avatar Arnaud Fontaine

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.
parent fc3da930
...@@ -7483,7 +7483,7 @@ class TestBusinessTemplate(BusinessTemplateMixin): ...@@ -7483,7 +7483,7 @@ class TestBusinessTemplate(BusinessTemplateMixin):
def stepSimulateToCreateNewRequest(self, sequence=None, **kw): 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: Why we need this is because:
- ZODB Components relies on this cache to prevend gc of the component, - ZODB Components relies on this cache to prevend gc of the component,
...@@ -7492,11 +7492,8 @@ class TestBusinessTemplate(BusinessTemplateMixin): ...@@ -7492,11 +7492,8 @@ class TestBusinessTemplate(BusinessTemplateMixin):
even if the request (transaction) is finished. even if the request (transaction) is finished.
This removal imitates the behavior in a real ZOPE environment This removal imitates the behavior in a real ZOPE environment
""" """
from Products.ERP5Type.Globals import get_request import erp5.component
request_obj = get_request() erp5.component.ref_manager.clear()
module_cache_set = getattr(request_obj, '_module_cache_set', None)
# delete the reference (decrement the reference count)
module_cache_set.clear()
def stepAddExtensionComponentToBusinessTemplate(self, sequence=None, **kw): def stepAddExtensionComponentToBusinessTemplate(self, sequence=None, **kw):
sequence['current_bt'].setProperty('template_extension_id_list', sequence['current_bt'].setProperty('template_extension_id_list',
......
...@@ -151,6 +151,7 @@ class ComponentTool(BaseTool): ...@@ -151,6 +151,7 @@ class ComponentTool(BaseTool):
if isinstance(package, ComponentDynamicPackage): if isinstance(package, ComponentDynamicPackage):
package.reset() package.reset()
erp5.component.ref_manager.gc()
if reset_portal_type_at_transaction_boundary: if reset_portal_type_at_transaction_boundary:
portal.portal_types.resetDynamicDocumentsOnceAtTransactionBoundary() portal.portal_types.resetDynamicDocumentsOnceAtTransactionBoundary()
else: else:
......
...@@ -21,6 +21,7 @@ ...@@ -21,6 +21,7 @@
############################################################################## ##############################################################################
# Load all monkey patches # Load all monkey patches
from Products.ERP5Type.patches import HTTPRequest
from Products.ERP5Type.patches import AccessControl_patch from Products.ERP5Type.patches import AccessControl_patch
from Products.ERP5Type.patches import Restricted from Products.ERP5Type.patches import Restricted
from Products.ERP5Type.patches import m2crypto from Products.ERP5Type.patches import m2crypto
......
...@@ -35,7 +35,6 @@ import imp ...@@ -35,7 +35,6 @@ import imp
import collections import collections
from Products.ERP5.ERP5Site import getSite from Products.ERP5.ERP5Site import getSite
from Products.ERP5Type.Globals import get_request
from . import aq_method_lock from . import aq_method_lock
from types import ModuleType from types import ModuleType
from zLOG import LOG, BLATHER, WARNING from zLOG import LOG, BLATHER, WARNING
...@@ -304,22 +303,8 @@ class ComponentDynamicPackage(ModuleType): ...@@ -304,22 +303,8 @@ class ComponentDynamicPackage(ModuleType):
if module_fullname_alias: if module_fullname_alias:
setattr(self, name, module) setattr(self, name, module)
# When the reference counter of a module reaches 0, its globals are all import erp5.component
# reset to None. So, if a thread performs a reset while another one erp5.component.ref_manager.add_module(module)
# 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)
return module return module
finally: finally:
......
...@@ -39,6 +39,91 @@ class PackageType(ModuleType): ...@@ -39,6 +39,91 @@ class PackageType(ModuleType):
""" """
__path__ = [] __path__ = []
class RefManager(dict):
"""
self[ComponentTool.last_sync] = (HTTP_REQUEST_WEAKSET,
COMPONENT_MODULE_SET)
"""
def add_request(self, request_obj):
from Products.ERP5Type.Tool.ComponentTool import last_sync
from weakref import WeakSet
# dict.setdefault() atomic from 2.7.3
self.setdefault(last_sync, (WeakSet(), set()))[0].add(request_obj)
def 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.add_request(get_request())
for (_, module_obj_set) in self.itervalues():
module_obj_set.add(module_obj)
def gc(self):
"""
Remove cache items with no Request Left.
"""
for (current_last_sync,
(request_obj_weakset, _)) in self.items():
if not request_obj_weakset:
del self[current_last_sync]
def clear(self):
"""
Completely clear the cache. Should never be called except to
simulate new Requests in Unit Tests for example.
"""
super(RefManager, self).clear()
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.ref_manager = RefManager()
class DynamicModule(ModuleType): class DynamicModule(ModuleType):
"""This module may generate new objects at runtime.""" """This module may generate new objects at runtime."""
# it's useful to have such a generic utility # it's useful to have such a generic utility
...@@ -130,7 +215,7 @@ def initializeDynamicModules(): ...@@ -130,7 +215,7 @@ def initializeDynamicModules():
loadTempPortalTypeClass) loadTempPortalTypeClass)
# ZODB Components # ZODB Components
erp5.component = PackageType("erp5.component") erp5.component = ComponentPackageType("erp5.component")
from component_package import ComponentDynamicPackage from component_package import ComponentDynamicPackage
......
# -*- 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.ref_manager.add_request(self)
HTTPRequest.__init__ = __init__
...@@ -83,12 +83,6 @@ class ERP5TypeLiveTestCase(ERP5TypeTestCaseMixin): ...@@ -83,12 +83,6 @@ class ERP5TypeLiveTestCase(ERP5TypeTestCaseMixin):
if self.portal is not None: if self.portal is not None:
return self.portal 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 from Products.ERP5.ERP5Site import getSite
site = getSite() site = getSite()
# reconstruct the acquistion chain with an independant request. # reconstruct the acquistion chain with an independant request.
...@@ -96,9 +90,6 @@ class ERP5TypeLiveTestCase(ERP5TypeTestCaseMixin): ...@@ -96,9 +90,6 @@ class ERP5TypeLiveTestCase(ERP5TypeTestCaseMixin):
from Testing.ZopeTestCase.utils import makerequest from Testing.ZopeTestCase.utils import makerequest
portal = getattr(makerequest(site.aq_parent), site.getId()) 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. # Make the various get_request patches return this request.
# This is for ERP5TypeTestCase patch # This is for ERP5TypeTestCase patch
from Testing.ZopeTestCase.connections import registry from Testing.ZopeTestCase.connections import registry
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment