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.

product/ERP5Type/Base.py

@@ -86,7 +86,6 @@ from Products.ERP5Type.Accessor.TypeDefinition import asDate
 from Products.ERP5Type.Message import Message
 from Products.ERP5Type.ConsistencyMessage import ConsistencyMessage
 from Products.ERP5Type.UnrestrictedMethod import UnrestrictedMethod
-from Products.ERP5Type.dynamic.import_lock import ImportLock
 
 from zope.interface import classImplementsOnly, implementedBy
 
@@ -711,7 +710,6 @@ class Base( CopyContainer,
   isTempDocument = ConstantGetter('isTempDocument', value=False)
 
   # Dynamic method acquisition system (code generation)
-  aq_method_lock = ImportLock()
   aq_method_generated = set()
   aq_method_generating = []
   aq_portal_type = {}

product/ERP5Type/Tool/ComponentTool.py

@@ -37,7 +37,7 @@ from AccessControl import ClassSecurityInfo
 from Products.ERP5Type import Permissions
 from AccessControl.Permission import Permission
 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 zLOG import LOG, INFO, WARNING
@@ -147,7 +147,7 @@ class ComponentTool(BaseTool):
     # class when Components are reset through aq_method_lock
     import erp5.component
     from Products.ERP5Type.dynamic.component_package import ComponentDynamicPackage
-    with Base.aq_method_lock:
+    with aq_method_lock:
       for package in erp5.component.__dict__.itervalues():
         if isinstance(package, ComponentDynamicPackage):
           package.reset()

product/ERP5Type/dynamic/__init__.py

+import threading
+aq_method_lock = threading.RLock()

product/ERP5Type/dynamic/component_package.py

@@ -31,8 +31,10 @@
 from __future__ import absolute_import
 
 import sys
+import imp
 
 from Products.ERP5.ERP5Site import getSite
+from . import aq_method_lock
 from types import ModuleType
 from zLOG import LOG, BLATHER
 
@@ -42,8 +44,6 @@ class ComponentVersionPackage(ModuleType):
   """
   __path__ = []
 
-from Products.ERP5Type.dynamic.import_lock import ImportLock
-
 class ComponentDynamicPackage(ModuleType):
   """
   A top-level component is a package as it contains modules, this is required
@@ -65,7 +65,6 @@ class ComponentDynamicPackage(ModuleType):
   # Necessary otherwise imports will fail because an object is considered a
   # package only if __path__ is defined
   __path__ = []
-  __lock = ImportLock()
 
   def __init__(self, namespace, portal_type):
     super(ComponentDynamicPackage, self).__init__(namespace)
@@ -108,24 +107,23 @@ class ComponentDynamicPackage(ModuleType):
       # 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
       # as it lags behind and depends upon objects being reindexed
-      with self.__lock:
-        for component in component_tool.objectValues(portal_type=self._portal_type):
-          # Only consider modified or validated states as state transition will
-          # be handled by component_validation_workflow which will take care of
-          # updating the registry
-          try:
-            validation_state_tuple = component.getValidationState()
-          except AttributeError:
-            # XXX: Accessors may have not been generated yet
-            pass
-          else:
-            if validation_state_tuple in ('modified', 'validated'):
-              version = component.getVersion(validated_only=True)
-              # The versions should have always been set on ERP5Site property
-              # beforehand
-              if version in version_priority_set:
-                reference = component.getReference(validated_only=True)
-                self.__registry_dict.setdefault(reference, {})[version] = component.getId()
+      for component in component_tool.objectValues(portal_type=self._portal_type):
+        # Only consider modified or validated states as state transition will
+        # be handled by component_validation_workflow which will take care of
+        # updating the registry
+        try:
+          validation_state_tuple = component.getValidationState()
+        except AttributeError:
+          # XXX: Accessors may have not been generated yet
+          pass
+        else:
+          if validation_state_tuple in ('modified', 'validated'):
+            version = component.getVersion(validated_only=True)
+            # The versions should have always been set on ERP5Site property
+            # beforehand
+            if version in version_priority_set:
+              reference = component.getReference(validated_only=True)
+              self.__registry_dict.setdefault(reference, {})[version] = component.getId()
 
     return self.__registry_dict
 
@@ -159,32 +157,53 @@ class ComponentDynamicPackage(ModuleType):
     if path or not fullname.startswith(self._namespace_prefix):
       return None
 
-    site = getSite()
-
-    # __import__ will first try a relative import, for example
-    # erp5.component.XXX.YYY.ZZZ where erp5.component.XXX.YYY is the current
-    # Component where an import is done
-    name = fullname[len(self._namespace_prefix):]
-    if '.' in name:
-      try:
-        version, name = name.split('.')
-        version = version[:-self.__version_suffix_len]
-      except ValueError:
+    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()
+
+      # __import__ will first try a relative import, for example
+      # erp5.component.XXX.YYY.ZZZ where erp5.component.XXX.YYY is the current
+      # Component where an import is done
+      name = fullname[len(self._namespace_prefix):]
+      if '.' in name:
+        try:
+          version, name = name.split('.')
+          version = version[:-self.__version_suffix_len]
+        except ValueError:
+          return None
+
+        try:
+          self._registry_dict[name][version]
+        except KeyError:
+          return None
+
+      # Skip unavailable components, otherwise Products for example could be
+      # wrongly considered as importable and thus the actual filesystem class
+      # ignored
+      elif (name not in self._registry_dict and
+            name[:-self.__version_suffix_len] not in site.getVersionPriorityNameList()):
         return None
 
-      try:
-        self._registry_dict[name][version]
-      except KeyError:
-        return None
+      return self
 
-    # Skip unavailable components, otherwise Products for example could be
-    # wrongly considered as importable and thus the actual filesystem class
-    # ignored
-    elif (name not in self._registry_dict and
-          name[:-self.__version_suffix_len] not in site.getVersionPriorityNameList()):
-      return None
+    finally:
+      aq_method_lock.release()
 
-    return self
+      # 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):
     """
@@ -220,106 +239,147 @@ class ComponentDynamicPackage(ModuleType):
     As per PEP-302, raise an ImportError if the Loader could not load the
     module for any reason...
     """
-    site = getSite()
-    name = fullname[len(self._namespace_prefix):]
-
-    # if only Version package (erp5.component.XXX.VERSION_version) is
-    # requested to be loaded, then create it if necessary
-    if name.endswith('_version'):
-      version = name[:-self.__version_suffix_len]
-      return (version in site.getVersionPriorityNameList() and
-              self._getVersionPackage(version) or None)
-
-    module_fullname_alias = None
-    version_package_name = name[:-self.__version_suffix_len]
-
-    # If a specific version of the Component has been requested
-    if '.' in name:
-      try:
-        version, name = name.split('.')
-        version = version[:-self.__version_suffix_len]
-      except ValueError, error:
-        raise ImportError("%s: should be %s.VERSION.COMPONENT_REFERENCE (%s)" % \
-                            (fullname, self._namespace, error))
-
-      try:
-        component_id = self._registry_dict[name][version]
-      except KeyError:
-        raise ImportError("%s: version %s of Component %s could not be found" % \
-                            (fullname, version, name))
-
-    # Otherwise, find the Component with the highest version priority
-    else:
-      try:
-        component_version_dict = self._registry_dict[name]
-      except KeyError:
-        raise ImportError("%s: Component %s could not be found" % (fullname,
-                                                                   name))
-
-      # Version priority name list is ordered in descending order
-      for version in site.getVersionPriorityNameList():
-        component_id = component_version_dict.get(version)
-        if component_id is not None:
-          break
-      else:
-        raise ImportError("%s: no version of Component %s in Site priority" % \
-                            (fullname, name))
+    # 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
 
-      # Check whether this module has already been loaded before for a
-      # specific version, if so, just add it to the upper level
-      try:
-        module = getattr(getattr(self, version + '_version'), name)
-      except AttributeError:
-        pass
+    try:
+      site = getSite()
+      name = fullname[len(self._namespace_prefix):]
+
+      # if only Version package (erp5.component.XXX.VERSION_version) is
+      # requested to be loaded, then create it if necessary
+      if name.endswith('_version'):
+        version = name[:-self.__version_suffix_len]
+        return (version in site.getVersionPriorityNameList() and
+                self._getVersionPackage(version) or None)
+
+      module_fullname_alias = None
+      version_package_name = name[:-self.__version_suffix_len]
+
+      # If a specific version of the Component has been requested
+      if '.' in name:
+        try:
+          version, name = name.split('.')
+          version = version[:-self.__version_suffix_len]
+        except ValueError, error:
+          raise ImportError("%s: should be %s.VERSION.COMPONENT_REFERENCE (%s)" % \
+                              (fullname, self._namespace, error))
+
+        try:
+          component_id = self._registry_dict[name][version]
+        except KeyError:
+          raise ImportError("%s: version %s of Component %s could not be found" % \
+                              (fullname, version, name))
+
+      # Otherwise, find the Component with the highest version priority
       else:
-        setattr(self, name, module)
-        return module
-
-      module_fullname_alias = self._namespace + '.' + name
-
-    component = getattr(site.portal_components, component_id)
-
-    module_fullname = '%s.%s_version.%s' % (self._namespace, version, name)
-    module = ModuleType(module_fullname, component.getDescription())
-
-    # The module *must* be in sys.modules before executing the code in case
-    # the module code imports (directly or indirectly) itself (see PEP 302)
-    sys.modules[module_fullname] = module
-    if module_fullname_alias:
-      sys.modules[module_fullname_alias] = module
-
-    # This must be set for imports at least (see PEP 302)
-    module.__file__ = '<' + component.getId() + '>'
-
+        try:
+          component_version_dict = self._registry_dict[name]
+        except KeyError:
+          raise ImportError("%s: Component %s could not be found" % (fullname,
+                                                                     name))
+
+        # Version priority name list is ordered in descending order
+        for version in site.getVersionPriorityNameList():
+          component_id = component_version_dict.get(version)
+          if component_id is not None:
+            break
+        else:
+          raise ImportError("%s: no version of Component %s in Site priority" % \
+                              (fullname, name))
+
+        # Check whether this module has already been loaded before for a
+        # specific version, if so, just add it to the upper level
+        try:
+          module = getattr(getattr(self, version + '_version'), name)
+        except AttributeError:
+          pass
+        else:
+          setattr(self, name, module)
+          return module
+
+        module_fullname_alias = self._namespace + '.' + name
+
+      component = getattr(site.portal_components, component_id)
+
+      module_fullname = '%s.%s_version.%s' % (self._namespace, version, name)
+      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:
-      component.load(module.__dict__, validated_only=True)
-    except Exception, error:
-      del sys.modules[module_fullname]
+      # The module *must* be in sys.modules before executing the code in case
+      # the module code imports (directly or indirectly) itself (see PEP 302)
+      sys.modules[module_fullname] = module
       if module_fullname_alias:
-        del sys.modules[module_fullname_alias]
+        sys.modules[module_fullname_alias] = module
 
-      raise ImportError("%s: cannot load Component %s (%s)" % (fullname,
-                                                               name,
-                                                               error))
+      # This must be set for imports at least (see PEP 302)
+      module.__file__ = '<' + component.getId() + '>'
 
-    module.__path__ = []
-    module.__loader__ = self
-    module.__name__ = module_fullname
-
-    # Add the newly created module to the Version package and add it as an
-    # alias to the top-level package as well
-    setattr(self._getVersionPackage(version), name, module)
-    if module_fullname_alias:
-      setattr(self, name, module)
+      try:
+        # 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:
+        del sys.modules[module_fullname]
+        if module_fullname_alias:
+          del sys.modules[module_fullname_alias]
+
+        raise ImportError("%s: cannot load Component %s (%s)" % (fullname,
+                                                                 name,
+                                                                 error))
+
+      module.__path__ = []
+      module.__loader__ = self
+      module.__name__ = module_fullname
+
+      # Add the newly created module to the Version package and add it as an
+      # alias to the top-level package as well
+      setattr(version_package, name, module)
+      if module_fullname_alias:
+        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):
     """
     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
     """
-    with self.__lock:
+    with aq_method_lock:
       return self.__load_module(fullname)
 
   def reset(self, sub_package=None):

product/ERP5Type/dynamic/dynamic_module.py

@@ -28,8 +28,8 @@
 ##############################################################################
 
 from types import ModuleType
+from . import aq_method_lock
 import sys
-from Products.ERP5Type.dynamic.import_lock import ImportLock
 
 class DynamicModule(ModuleType):
   """This module may generate new objects at runtime."""
@@ -41,13 +41,13 @@ class DynamicModule(ModuleType):
   def __init__(self, name, factory, doc=None):
     super(DynamicModule, self).__init__(name, doc=doc)
     self._factory = factory
-    self._lock = ImportLock()
 
   def __getattr__(self, name):
     if name[:2] == '__':
       raise AttributeError('%r module has no attribute %r'
                            % (self.__name__, name))
-    with self._lock:
+
+    with aq_method_lock:
       try:
         return super(DynamicModule, self).__getattribute__(name)
       except AttributeError:

product/ERP5Type/dynamic/import_lock.py

-# -*- 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()

product/ERP5Type/dynamic/lazy_class.py

@@ -6,6 +6,7 @@ from Products.ERP5Type import Permissions
 from Products.ERP5Type.Accessor.Constant import Getter as ConstantGetter
 from Products.ERP5Type.Globals import InitializeClass
 from Products.ERP5Type.Base import Base as ERP5Base
+from . import aq_method_lock
 from Products.ERP5Type.Base import PropertyHolder, initializePortalTypeDynamicWorkflowMethods
 from Products.ERP5Type.Utils import UpperCase
 from Products.ERP5Type.Core.CategoryProperty import CategoryProperty
@@ -319,7 +320,7 @@ class PortalTypeMetaClass(GhostBaseMetaClass, PropertyHolder):
     portal_type = klass.__name__
     from Products.ERP5.ERP5Site import getSite
     site = getSite()
-    ERP5Base.aq_method_lock.acquire()
+    aq_method_lock.acquire()
     try:
       try:
         class_definition = generatePortalTypeClass(site, portal_type)
@@ -363,7 +364,7 @@ class PortalTypeMetaClass(GhostBaseMetaClass, PropertyHolder):
       import traceback; traceback.print_exc()
       raise
     finally:
-      ERP5Base.aq_method_lock.release()
+      aq_method_lock.release()
 
 def generateLazyPortalTypeClass(portal_type_name):
   return PortalTypeMetaClass(portal_type_name,

product/ERP5Type/dynamic/portal_type_class.py

@@ -33,7 +33,8 @@ import inspect
 import transaction
 
 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.Utils import setDefaultClassProperties
 from Products.ERP5Type import document_class_registry, mixin_class_registry
@@ -318,7 +319,7 @@ def synchronizeDynamicModules(context, force=False):
     last_sync = cookie
 
   import erp5
-  with Base.aq_method_lock:
+  with aq_method_lock:
     # Thanks to TransactionalResource, the '_bootstrapped' global variable
     # is updated in a transactional way. Without it, it would be required to
     # restart the instance if anything went wrong.

product/ERP5Type/mixin/component.py

@@ -310,6 +310,9 @@ class ComponentMixin(PropertyRecordableMixin, Base):
     Initially, namespace_dict default parameter value was an empty dict to
     allow checking the source code before validate, but this is completely
     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:
       text_content = self.getTextContent(validated_only=validated_only)