Commit 3181d5ad authored by Arnaud Fontaine's avatar Arnaud Fontaine

ZODB Components: Fix deadlock on import lock.

Since a29456bc, only import lock is used instead of aq_method_lock, dynamic
modules locks and import lock. However, it was creating another deadlock:

when the import lock is held in one thread (for instance when trying to
perform migration to Portal Type as Classes and ZODB Property Sheets), and an
object is loaded from ZODB, a request is sent to ZEO, and this blocks until
another thread (asyncore) gets the ZEO reply and sends it back to the first
thread.

However, if the asyncore thread receives an Exception, it will tries to import
its module and thus create a deadlock as the import lock is still held by the
first thread.
parent e8d1f578
...@@ -86,7 +86,6 @@ from Products.ERP5Type.Accessor.TypeDefinition import asDate ...@@ -86,7 +86,6 @@ from Products.ERP5Type.Accessor.TypeDefinition import asDate
from Products.ERP5Type.Message import Message from Products.ERP5Type.Message import Message
from Products.ERP5Type.ConsistencyMessage import ConsistencyMessage from Products.ERP5Type.ConsistencyMessage import ConsistencyMessage
from Products.ERP5Type.UnrestrictedMethod import UnrestrictedMethod from Products.ERP5Type.UnrestrictedMethod import UnrestrictedMethod
from Products.ERP5Type.dynamic.import_lock import ImportLock
from zope.interface import classImplementsOnly, implementedBy from zope.interface import classImplementsOnly, implementedBy
...@@ -720,7 +719,6 @@ class Base( CopyContainer, ...@@ -720,7 +719,6 @@ class Base( CopyContainer,
isTempDocument = ConstantGetter('isTempDocument', value=False) isTempDocument = ConstantGetter('isTempDocument', value=False)
# Dynamic method acquisition system (code generation) # Dynamic method acquisition system (code generation)
aq_method_lock = ImportLock()
aq_method_generated = set() aq_method_generated = set()
aq_method_generating = [] aq_method_generating = []
aq_portal_type = {} aq_portal_type = {}
......
...@@ -37,7 +37,7 @@ from AccessControl import ClassSecurityInfo ...@@ -37,7 +37,7 @@ from AccessControl import ClassSecurityInfo
from Products.ERP5Type import Permissions from Products.ERP5Type import Permissions
from AccessControl.Permission import Permission from AccessControl.Permission import Permission
from Products.ERP5Type.Tool.BaseTool import BaseTool from Products.ERP5Type.Tool.BaseTool import BaseTool
from Products.ERP5Type.Base import Base from Products.ERP5Type.dynamic import aq_method_lock
from Products.ERP5Type.TransactionalVariable import getTransactionalVariable from Products.ERP5Type.TransactionalVariable import getTransactionalVariable
from zLOG import LOG, INFO, WARNING from zLOG import LOG, INFO, WARNING
...@@ -147,7 +147,7 @@ class ComponentTool(BaseTool): ...@@ -147,7 +147,7 @@ class ComponentTool(BaseTool):
# class when Components are reset through aq_method_lock # class when Components are reset through aq_method_lock
import erp5.component import erp5.component
from Products.ERP5Type.dynamic.component_package import ComponentDynamicPackage from Products.ERP5Type.dynamic.component_package import ComponentDynamicPackage
with Base.aq_method_lock: with aq_method_lock:
for package in erp5.component.__dict__.itervalues(): for package in erp5.component.__dict__.itervalues():
if isinstance(package, ComponentDynamicPackage): if isinstance(package, ComponentDynamicPackage):
package.reset() package.reset()
......
import threading
aq_method_lock = threading.RLock()
...@@ -31,8 +31,10 @@ ...@@ -31,8 +31,10 @@
from __future__ import absolute_import from __future__ import absolute_import
import sys import sys
import imp
from Products.ERP5.ERP5Site import getSite from Products.ERP5.ERP5Site import getSite
from . import aq_method_lock
from types import ModuleType from types import ModuleType
from zLOG import LOG, BLATHER from zLOG import LOG, BLATHER
...@@ -42,8 +44,6 @@ class ComponentVersionPackage(ModuleType): ...@@ -42,8 +44,6 @@ class ComponentVersionPackage(ModuleType):
""" """
__path__ = [] __path__ = []
from Products.ERP5Type.dynamic.import_lock import ImportLock
class ComponentDynamicPackage(ModuleType): class ComponentDynamicPackage(ModuleType):
""" """
A top-level component is a package as it contains modules, this is required A top-level component is a package as it contains modules, this is required
...@@ -65,7 +65,6 @@ class ComponentDynamicPackage(ModuleType): ...@@ -65,7 +65,6 @@ class ComponentDynamicPackage(ModuleType):
# Necessary otherwise imports will fail because an object is considered a # Necessary otherwise imports will fail because an object is considered a
# package only if __path__ is defined # package only if __path__ is defined
__path__ = [] __path__ = []
__lock = ImportLock()
def __init__(self, namespace, portal_type): def __init__(self, namespace, portal_type):
super(ComponentDynamicPackage, self).__init__(namespace) super(ComponentDynamicPackage, self).__init__(namespace)
...@@ -108,7 +107,6 @@ class ComponentDynamicPackage(ModuleType): ...@@ -108,7 +107,6 @@ class ComponentDynamicPackage(ModuleType):
# objectValues should not be used for a large number of objects, but # objectValues should not be used for a large number of objects, but
# this is only done upon reset, moreover using the Catalog is too risky # this is only done upon reset, moreover using the Catalog is too risky
# as it lags behind and depends upon objects being reindexed # as it lags behind and depends upon objects being reindexed
with self.__lock:
for component in component_tool.objectValues(portal_type=self._portal_type): for component in component_tool.objectValues(portal_type=self._portal_type):
# Only consider modified or validated states as state transition will # Only consider modified or validated states as state transition will
# be handled by component_validation_workflow which will take care of # be handled by component_validation_workflow which will take care of
...@@ -159,6 +157,19 @@ class ComponentDynamicPackage(ModuleType): ...@@ -159,6 +157,19 @@ class ComponentDynamicPackage(ModuleType):
if path or not fullname.startswith(self._namespace_prefix): if path or not fullname.startswith(self._namespace_prefix):
return None return None
import_lock_held = True
try:
imp.release_lock()
except RuntimeError:
import_lock_held = False
# The import lock has been released, but as _registry_dict may be
# initialized or cleared, no other Components should access this critical
# region
#
# TODO-arnau: Too coarse-grain?
aq_method_lock.acquire()
try:
site = getSite() site = getSite()
# __import__ will first try a relative import, for example # __import__ will first try a relative import, for example
...@@ -186,6 +197,14 @@ class ComponentDynamicPackage(ModuleType): ...@@ -186,6 +197,14 @@ class ComponentDynamicPackage(ModuleType):
return self return self
finally:
aq_method_lock.release()
# Internal release of import lock at the end of import machinery will
# fail if the hook is not acquired
if import_lock_held:
imp.acquire_lock()
def _getVersionPackage(self, version): def _getVersionPackage(self, version):
""" """
Get the version package (NAMESPACE.VERSION_version) for the given version Get the version package (NAMESPACE.VERSION_version) for the given version
...@@ -220,6 +239,25 @@ class ComponentDynamicPackage(ModuleType): ...@@ -220,6 +239,25 @@ class ComponentDynamicPackage(ModuleType):
As per PEP-302, raise an ImportError if the Loader could not load the As per PEP-302, raise an ImportError if the Loader could not load the
module for any reason... module for any reason...
""" """
# In Python < 3.3, the import lock is a global lock for all modules:
# http://bugs.python.org/issue9260
#
# So, release the import lock acquired by import statement on all hooks to
# load objects from ZODB. When an object is requested from ZEO, it sends a
# RPC request and lets the asyncore thread gets the reply. This reply may
# be a tuple (PICKLE, TID), sent directly to the first thread, or an
# Exception, which tries to import a ZODB module and thus creates a
# deadlock because of the global import lock
#
# Also, handle the case where find_module() may be called without import
# statement as it does change anything in sys.modules
import_lock_held = True
try:
imp.release_lock()
except RuntimeError:
import_lock_held = False
try:
site = getSite() site = getSite()
name = fullname[len(self._namespace_prefix):] name = fullname[len(self._namespace_prefix):]
...@@ -282,6 +320,20 @@ class ComponentDynamicPackage(ModuleType): ...@@ -282,6 +320,20 @@ class ComponentDynamicPackage(ModuleType):
module_fullname = '%s.%s_version.%s' % (self._namespace, version, name) module_fullname = '%s.%s_version.%s' % (self._namespace, version, name)
module = ModuleType(module_fullname, component.getDescription()) module = ModuleType(module_fullname, component.getDescription())
source_code_str = component.getTextContent(validated_only=True)
version_package = self._getVersionPackage(version)
finally:
# Internal release of import lock at the end of import machinery will
# fail if the hook is not acquired
if import_lock_held:
imp.acquire_lock()
# All the required objects have been loaded, acquire import lock to modify
# sys.modules and execute PEP302 requisites
if not import_lock_held:
imp.acquire_lock()
try:
# The module *must* be in sys.modules before executing the code in case # The module *must* be in sys.modules before executing the code in case
# the module code imports (directly or indirectly) itself (see PEP 302) # the module code imports (directly or indirectly) itself (see PEP 302)
sys.modules[module_fullname] = module sys.modules[module_fullname] = module
...@@ -292,7 +344,9 @@ class ComponentDynamicPackage(ModuleType): ...@@ -292,7 +344,9 @@ class ComponentDynamicPackage(ModuleType):
module.__file__ = '<' + component.getId() + '>' module.__file__ = '<' + component.getId() + '>'
try: try:
component.load(module.__dict__, validated_only=True) # XXX: Any loading from ZODB while exec'ing the source code will result
# in a deadlock
exec source_code_str in module.__dict__
except Exception, error: except Exception, error:
del sys.modules[module_fullname] del sys.modules[module_fullname]
if module_fullname_alias: if module_fullname_alias:
...@@ -308,18 +362,24 @@ class ComponentDynamicPackage(ModuleType): ...@@ -308,18 +362,24 @@ class ComponentDynamicPackage(ModuleType):
# Add the newly created module to the Version package and add it as an # Add the newly created module to the Version package and add it as an
# alias to the top-level package as well # alias to the top-level package as well
setattr(self._getVersionPackage(version), name, module) setattr(version_package, name, module)
if module_fullname_alias: if module_fullname_alias:
setattr(self, name, module) setattr(self, name, module)
return module return module
finally:
# load_module() can be called outside of import machinery, for example
# to check first if the module can be handled by Component and then try
# to load it without going through the same code again
if not import_lock_held:
imp.release_lock()
def load_module(self, fullname): def load_module(self, fullname):
""" """
Make sure that loading module is thread-safe using aq_method_lock to make Make sure that loading module is thread-safe using aq_method_lock to make
sure that modules do not disappear because of an ongoing reset sure that modules do not disappear because of an ongoing reset
""" """
with self.__lock: with aq_method_lock:
return self.__load_module(fullname) return self.__load_module(fullname)
def reset(self, sub_package=None): def reset(self, sub_package=None):
......
...@@ -28,8 +28,8 @@ ...@@ -28,8 +28,8 @@
############################################################################## ##############################################################################
from types import ModuleType from types import ModuleType
from . import aq_method_lock
import sys import sys
from Products.ERP5Type.dynamic.import_lock import ImportLock
class DynamicModule(ModuleType): class DynamicModule(ModuleType):
"""This module may generate new objects at runtime.""" """This module may generate new objects at runtime."""
...@@ -41,13 +41,13 @@ class DynamicModule(ModuleType): ...@@ -41,13 +41,13 @@ class DynamicModule(ModuleType):
def __init__(self, name, factory, doc=None): def __init__(self, name, factory, doc=None):
super(DynamicModule, self).__init__(name, doc=doc) super(DynamicModule, self).__init__(name, doc=doc)
self._factory = factory self._factory = factory
self._lock = ImportLock()
def __getattr__(self, name): def __getattr__(self, name):
if name[:2] == '__': if name[:2] == '__':
raise AttributeError('%r module has no attribute %r' raise AttributeError('%r module has no attribute %r'
% (self.__name__, name)) % (self.__name__, name))
with self._lock:
with aq_method_lock:
try: try:
return super(DynamicModule, self).__getattribute__(name) return super(DynamicModule, self).__getattribute__(name)
except AttributeError: except AttributeError:
......
# -*- coding: utf-8 -*-
##############################################################################
# Copyright (c) 2012 Nexedi SA and Contributors. All Rights Reserved.
#
# WARNING: This program as such is intended to be used by professional
# programmers who take the whole responsibility of assessing all potential
# consequences resulting from its eventual inadequacies and bugs
# End users who are looking for a ready-to-use solution with commercial
# guarantees and support are strongly advised to contract a Free Software
# Service Company
#
# This program is Free Software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
##############################################################################
import imp
class ImportLock(object):
"""
This class provides the interpreter's import lock.
It is intended to use in ERP5Type.dynamic to avoid possible dead lock.
It can be used in two ways :
1) 'with' statement
lock = ImportLock()
with lock:
...
2) traditional 'try' and 'finally'
lock = ImportLock()
lock.acquire()
try:
...
finally:
lock.release()
"""
def __enter__(self):
imp.acquire_lock()
def __exit__(self, type, value, tb):
imp.release_lock()
def acquire(self):
imp.acquire_lock()
def release(self):
imp.release_lock()
...@@ -6,6 +6,7 @@ from Products.ERP5Type import Permissions ...@@ -6,6 +6,7 @@ from Products.ERP5Type import Permissions
from Products.ERP5Type.Accessor.Constant import Getter as ConstantGetter from Products.ERP5Type.Accessor.Constant import Getter as ConstantGetter
from Products.ERP5Type.Globals import InitializeClass from Products.ERP5Type.Globals import InitializeClass
from Products.ERP5Type.Base import Base as ERP5Base from Products.ERP5Type.Base import Base as ERP5Base
from . import aq_method_lock
from Products.ERP5Type.Base import PropertyHolder, initializePortalTypeDynamicWorkflowMethods from Products.ERP5Type.Base import PropertyHolder, initializePortalTypeDynamicWorkflowMethods
from Products.ERP5Type.Utils import UpperCase from Products.ERP5Type.Utils import UpperCase
from Products.ERP5Type.Core.CategoryProperty import CategoryProperty from Products.ERP5Type.Core.CategoryProperty import CategoryProperty
...@@ -319,7 +320,7 @@ class PortalTypeMetaClass(GhostBaseMetaClass, PropertyHolder): ...@@ -319,7 +320,7 @@ class PortalTypeMetaClass(GhostBaseMetaClass, PropertyHolder):
portal_type = klass.__name__ portal_type = klass.__name__
from Products.ERP5.ERP5Site import getSite from Products.ERP5.ERP5Site import getSite
site = getSite() site = getSite()
ERP5Base.aq_method_lock.acquire() aq_method_lock.acquire()
try: try:
try: try:
class_definition = generatePortalTypeClass(site, portal_type) class_definition = generatePortalTypeClass(site, portal_type)
...@@ -363,7 +364,7 @@ class PortalTypeMetaClass(GhostBaseMetaClass, PropertyHolder): ...@@ -363,7 +364,7 @@ class PortalTypeMetaClass(GhostBaseMetaClass, PropertyHolder):
import traceback; traceback.print_exc() import traceback; traceback.print_exc()
raise raise
finally: finally:
ERP5Base.aq_method_lock.release() aq_method_lock.release()
def generateLazyPortalTypeClass(portal_type_name): def generateLazyPortalTypeClass(portal_type_name):
return PortalTypeMetaClass(portal_type_name, return PortalTypeMetaClass(portal_type_name,
......
...@@ -33,7 +33,8 @@ import inspect ...@@ -33,7 +33,8 @@ import inspect
import transaction import transaction
from Products.ERP5Type.mixin.temporary import TemporaryDocumentMixin from Products.ERP5Type.mixin.temporary import TemporaryDocumentMixin
from Products.ERP5Type.Base import Base, resetRegisteredWorkflowMethod from Products.ERP5Type.Base import resetRegisteredWorkflowMethod
from . import aq_method_lock
from Products.ERP5Type.Globals import InitializeClass from Products.ERP5Type.Globals import InitializeClass
from Products.ERP5Type.Utils import setDefaultClassProperties from Products.ERP5Type.Utils import setDefaultClassProperties
from Products.ERP5Type import document_class_registry, mixin_class_registry from Products.ERP5Type import document_class_registry, mixin_class_registry
...@@ -318,7 +319,7 @@ def synchronizeDynamicModules(context, force=False): ...@@ -318,7 +319,7 @@ def synchronizeDynamicModules(context, force=False):
last_sync = cookie last_sync = cookie
import erp5 import erp5
with Base.aq_method_lock: with aq_method_lock:
# Thanks to TransactionalResource, the '_bootstrapped' global variable # Thanks to TransactionalResource, the '_bootstrapped' global variable
# is updated in a transactional way. Without it, it would be required to # is updated in a transactional way. Without it, it would be required to
# restart the instance if anything went wrong. # restart the instance if anything went wrong.
......
...@@ -310,6 +310,9 @@ class ComponentMixin(PropertyRecordableMixin, Base): ...@@ -310,6 +310,9 @@ class ComponentMixin(PropertyRecordableMixin, Base):
Initially, namespace_dict default parameter value was an empty dict to Initially, namespace_dict default parameter value was an empty dict to
allow checking the source code before validate, but this is completely allow checking the source code before validate, but this is completely
wrong as the object reference is kept accross each call wrong as the object reference is kept accross each call
TODO-arnau: Not used anymore in component_package, so this could be
removed as soon as pyflakes is used instead
""" """
if text_content is None: if text_content is None:
text_content = self.getTextContent(validated_only=validated_only) text_content = self.getTextContent(validated_only=validated_only)
......
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