Speed up reset in __of__.

product/ERP5/ERP5Site.py

@@ -340,15 +340,15 @@ class ERP5Site(FolderMixIn, CMFSite, CacheCookieMixin):
       tv['ERP5Site.__of__'] = None
       setSite(self)
 
-      # If Components are reset, then portal type classes should be reset
       try:
-        reset_portal_type = self.portal_components.reset(force=False,
-                                                         reset_portal_type=False)
-      # This should only happen before erp5_core is installed
+        component_tool = self.portal_components
       except AttributeError:
-        reset_portal_type = False
+        # This should only happen before erp5_core is installed
+        synchronizeDynamicModules(self)
+      else:
+        # If Components are reset, then portal type classes should be reset
+        synchronizeDynamicModules(self, component_tool.reset())
 
-      synchronizeDynamicModules(self, force=True)
     return self
 
   def manage_beforeDelete(self, item, container):

product/ERP5Type/Tool/ComponentTool.py

@@ -56,7 +56,7 @@ class ComponentTool(BaseTool):
   security.declareObjectProtected(Permissions.AccessContentsInformation)
 
   security.declareProtected(Permissions.ResetDynamicClasses, 'reset')
-  def reset(self, force=True, reset_portal_type=True):
+  def reset(self, force=False, reset_portal_type=False):
     """
     XXX-arnau: global reset
     """
@@ -115,4 +115,5 @@ class ComponentTool(BaseTool):
     key = 'ComponentTool.resetOnceAtTransactionBoundary'
     if key not in tv:
       tv[key] = None
-      transaction.get().addBeforeCommitHook(self.reset)
+      transaction.get().addBeforeCommitHook(self.reset,
+                                            args=(True, True))

product/ERP5Type/tests/testDynamicClassGeneration.py

@@ -1254,7 +1254,7 @@ class _TestZodbComponent(SecurityTestCase):
     self._component_tool = self.getPortal().portal_components
     self._module = __import__(self._getComponentModuleName(),
                               fromlist=['erp5.component'])
-    self._component_tool.reset()
+    self._component_tool.reset(force=True, reset_portal_type=True)
 
   @abc.abstractmethod
   def _newComponent(self, reference, text_content, version='erp5'):
@@ -1362,7 +1362,7 @@ class _TestZodbComponent(SecurityTestCase):
       self.assertEquals(error_message, error_list[0])
       self.assertEquals(component.getReference(), invalid_reference)
       self.assertEquals(component.getReference(validated_only=True), valid_reference)
-      self._component_tool.reset()
+      self._component_tool.reset(force=True, reset_portal_type=True)
       self.assertModuleImportable(valid_reference)
 
     ComponentTool.reset = assertResetCalled
@@ -1430,7 +1430,7 @@ class _TestZodbComponent(SecurityTestCase):
       self.assertEquals(error_message, error_list[0])
       self.assertEquals(component.getVersion(), invalid_version)
       self.assertEquals(component.getVersion(validated_only=True), valid_version)
-      self._component_tool.reset()
+      self._component_tool.reset(force=True, reset_portal_type=True)
       self.assertModuleImportable(reference)
 
     ComponentTool.reset = assertResetCalled
@@ -1495,7 +1495,7 @@ class _TestZodbComponent(SecurityTestCase):
       self.assertTrue(error_list[0].startswith(error_message))
       self.assertEquals(component.getTextContent(), invalid_code)
       self.assertEquals(component.getTextContent(validated_only=True), valid_code)
-      self._component_tool.reset()
+      self._component_tool.reset(force=True, reset_portal_type=True)
       self.assertModuleImportable('TestComponentWithSyntaxError')
 
     ComponentTool.reset = assertResetCalled
@@ -1780,7 +1780,7 @@ class TestPortalType(Person):
     self.assertFalse(self._module.TestPortalType in person.__class__.mro())
 
     # Reset Portal Type classes to ghost to make sure that everything is reset
-    self._component_tool.reset()
+    self._component_tool.reset(force=True, reset_portal_type=True)
 
     # TestPortalType must be in available type class list
     self.assertTrue('TestPortalType' in person_type.getDocumentTypeList())

[Julien Muchembled]

There's a problem here: it may do too much, before migration code is called (I assume that synchronizeDynamicModules is the best place to migrate core things).

@bminusl currently has an issue with the removal of CMFDefault: it requires portal_membership to be upgraded but it fails very early at https://lab.nexedi.com/nexedi/erp5/blob/82206749afb1d03403e9bf1d2c036c6b5ed20fde/product/ERP5Type/Utils.py#L1269 (broken object).

[Arnaud Fontaine]

@jm How so? Although I'm bit doubtful considering that this has been here for 7 years without any problem so far, would you care providing a traceback or further details? Thanks.

[Bryton Lacquement]

> /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5/ERP5Site.py(430)__of__()
    429       try:
--> 430         component_tool = self.portal_components
    431       except AttributeError:

[... truncated ...]

> /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/Utils.py(1270)createExpressionContext()
   1269     import ipdb; ipdb.set_trace()
-> 1270     if pm is None or pm.isAnonymousUser():
   1271       member = None

ipdb> pm
<persistent broken Products.CMFDefault.MembershipTool.MembershipTool instance '\x00\x00\x00\x00\x00\x00\x00\xeb'>

The ERP5 in this case was instantiated with CMF 2.2, and CMFDefault was available. We are moving to CMF 2.3 and decided to drop CMFDefault. So now, we use MembershipTool from CMFCore instead of CMFDefault.

The problem is that we wrote code in synchronizeDynamicModules to properly migrate the tool, but this code is called too late: component_tool = self.portal_components runs code that requires a MembershipTool but it has not yet been migrated, and is thus broken.

[Bryton Lacquement]

Here's the full traceback:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5/ERP5Site.py(430)__of__()
    429       try:
--> 430         component_tool = self.portal_components
    431       except AttributeError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(113)__getattribute__()
    112       try:
--> 113         self.__class__.loadClass()
    114       except ConflictError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(350)loadClass()
    349       try:
--> 350         class_definition = generatePortalTypeClass(site, portal_type)
    351       except AttributeError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/portal_type_class.py(151)generatePortalTypeClass()
    150   # type provider other than Types Tool.
--> 151   portal_type = getattr(site.portal_types, portal_type_name, None)
    152 
  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(113)__getattribute__()
    112       try:
--> 113         self.__class__.loadClass()
    114       except ConflictError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(350)loadClass()
    349       try:
--> 350         class_definition = generatePortalTypeClass(site, portal_type)
    351       except AttributeError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/portal_type_class.py(151)generatePortalTypeClass()
    150   # type provider other than Types Tool.
--> 151   portal_type = getattr(site.portal_types, portal_type_name, None)
    152 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(113)__getattribute__()
    112       try:
--> 113         self.__class__.loadClass()
    114       except ConflictError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(350)loadClass()
    349       try:
--> 350         class_definition = generatePortalTypeClass(site, portal_type)
    351       except AttributeError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/portal_type_class.py(264)generatePortalTypeClass()
    263                                                          portal_type,
--> 264                                                          klass)
    265 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/accessor_holder.py(432)createAllAccessorHolderList()
    431                             getPropertySheetValueList(site,
--> 432                                                       property_sheet_name_set))
    433 
  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/accessor_holder.py(289)getPropertySheetValueList()
    288   try:
--> 289     property_sheet_tool = site.portal_property_sheets
    290 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(113)__getattribute__()
    112       try:
--> 113         self.__class__.loadClass()
    114       except ConflictError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(350)loadClass()
    349       try:
--> 350         class_definition = generatePortalTypeClass(site, portal_type)
    351       except AttributeError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/portal_type_class.py(264)generatePortalTypeClass()
    263                                                          portal_type,
--> 264                                                          klass)
    265 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/accessor_holder.py(432)createAllAccessorHolderList()
    431                             getPropertySheetValueList(site,
--> 432                                                       property_sheet_name_set))
    433 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/accessor_holder.py(289)getPropertySheetValueList()
    288   try:
--> 289     property_sheet_tool = site.portal_property_sheets
    290 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(113)__getattribute__()
    112       try:
--> 113         self.__class__.loadClass()
    114       except ConflictError:
  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(383)loadClass()
    382       if len(base_tuple) > 1:
--> 383         klass.generatePortalTypeAccessors(site, portal_type_category_list)
    384         # need to set %s__roles__ for generated methods

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(278)generatePortalTypeAccessors()
    277   def generatePortalTypeAccessors(cls, site, portal_type_category_list):
--> 278     category_tool = getattr(site, 'portal_categories', None)
    279     for category_id in portal_type_category_list:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(113)__getattribute__()
    112       try:
--> 113         self.__class__.loadClass()
    114       except ConflictError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(350)loadClass()
    349       try:
--> 350         class_definition = generatePortalTypeClass(site, portal_type)
    351       except AttributeError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/portal_type_class.py(218)generatePortalTypeClass()
    217         import erp5.component.tool
--> 218         module = erp5.component.tool.find_load_module(type_class)
    219 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/component_package.py(384)find_load_module()
    383       # returning 'erp5' (requiring fromlist parameter which is slower)
--> 384       return import_module(fullname)
    385     except ImportError as e:

  /srv/slapgrid/slappart6/srv/runner/software/04e02e55ae7bcf90dd4bd7e2450a773c/parts/python2.7/lib/python2.7/importlib/__init__.py(37)import_module()
     36         name = _resolve_name(name[level:], package, level)
---> 37     __import__(name)
     38     return sys.modules[name]
  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/component_package.py(165)find_module()
    164         try:
--> 165           component_tool = aq_base(site.portal_components)
    166         except AttributeError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(113)__getattribute__()
    112       try:
--> 113         self.__class__.loadClass()
    114       except ConflictError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(350)loadClass()
    349       try:
--> 350         class_definition = generatePortalTypeClass(site, portal_type)
    351       except AttributeError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/portal_type_class.py(264)generatePortalTypeClass()
    263                                                          portal_type,
--> 264                                                          klass)
    265 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/accessor_holder.py(432)createAllAccessorHolderList()
    431                             getPropertySheetValueList(site,
--> 432                                                       property_sheet_name_set))
    433 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/accessor_holder.py(303)getPropertySheetValueList()
    302     try:
--> 303       property_sheet = property_sheet_tool._getOb(property_sheet_name)
    304     except (AttributeError, KeyError):

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/Core/Folder.py(938)_getOb()
    937         raise KeyError(id)
--> 938     return folder._getOb(self, id, *args, **kw)
    939 
  /srv/slapgrid/slappart6/srv/runner/software/04e02e55ae7bcf90dd4bd7e2450a773c/eggs/Products.BTreeFolder2-2.13.5-py2.7.egg/Products/BTreeFolder2/BTreeFolder2.py(220)_getOb()
    219         try:
--> 220             return self._tree[id].__of__(self)
    221         except KeyError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(113)__getattribute__()
    112       try:
--> 113         self.__class__.loadClass()
    114       except ConflictError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(350)loadClass()
    349       try:
--> 350         class_definition = generatePortalTypeClass(site, portal_type)
    351       except AttributeError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/portal_type_class.py(264)generatePortalTypeClass()
    263                                                          portal_type,
--> 264                                                          klass)
    265 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/accessor_holder.py(432)createAllAccessorHolderList()
    431                             getPropertySheetValueList(site,
--> 432                                                       property_sheet_name_set))
    433 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/accessor_holder.py(303)getPropertySheetValueList()
    302     try:
--> 303       property_sheet = property_sheet_tool._getOb(property_sheet_name)
    304     except (AttributeError, KeyError):

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/Core/Folder.py(938)_getOb()
    937         raise KeyError(id)
--> 938     return folder._getOb(self, id, *args, **kw)
    939 
  /srv/slapgrid/slappart6/srv/runner/software/04e02e55ae7bcf90dd4bd7e2450a773c/eggs/Products.BTreeFolder2-2.13.5-py2.7.egg/Products/BTreeFolder2/BTreeFolder2.py(220)_getOb()
    219         try:
--> 220             return self._tree[id].__of__(self)
    221         except KeyError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(113)__getattribute__()
    112       try:
--> 113         self.__class__.loadClass()
    114       except ConflictError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(383)loadClass()
    382       if len(base_tuple) > 1:
--> 383         klass.generatePortalTypeAccessors(site, portal_type_category_list)
    384         # need to set %s__roles__ for generated methods

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(278)generatePortalTypeAccessors()
    277   def generatePortalTypeAccessors(cls, site, portal_type_category_list):
--> 278     category_tool = getattr(site, 'portal_categories', None)
    279     for category_id in portal_type_category_list:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(113)__getattribute__()
    112       try:
--> 113         self.__class__.loadClass()
    114       except ConflictError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(350)loadClass()
    349       try:
--> 350         class_definition = generatePortalTypeClass(site, portal_type)
    351       except AttributeError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/portal_type_class.py(218)generatePortalTypeClass()
    217         import erp5.component.tool
--> 218         module = erp5.component.tool.find_load_module(type_class)
    219 
  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/component_package.py(384)find_load_module()
    383       # returning 'erp5' (requiring fromlist parameter which is slower)
--> 384       return import_module(fullname)
    385     except ImportError as e:

  /srv/slapgrid/slappart6/srv/runner/software/04e02e55ae7bcf90dd4bd7e2450a773c/parts/python2.7/lib/python2.7/importlib/__init__.py(37)import_module()
     36         name = _resolve_name(name[level:], package, level)
---> 37     __import__(name)
     38     return sys.modules[name]

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/component_package.py(165)find_module()
    164         try:
--> 165           component_tool = aq_base(site.portal_components)
    166         except AttributeError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(113)__getattribute__()
    112       try:
--> 113         self.__class__.loadClass()
    114       except ConflictError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(383)loadClass()
    382       if len(base_tuple) > 1:
--> 383         klass.generatePortalTypeAccessors(site, portal_type_category_list)
    384         # need to set %s__roles__ for generated methods

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(278)generatePortalTypeAccessors()
    277   def generatePortalTypeAccessors(cls, site, portal_type_category_list):
--> 278     category_tool = getattr(site, 'portal_categories', None)
    279     for category_id in portal_type_category_list:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(113)__getattribute__()
    112       try:
--> 113         self.__class__.loadClass()
    114       except ConflictError:
  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/lazy_class.py(350)loadClass()
    349       try:
--> 350         class_definition = generatePortalTypeClass(site, portal_type)
    351       except AttributeError:

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/portal_type_class.py(264)generatePortalTypeClass()
    263                                                          portal_type,
--> 264                                                          klass)
    265 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/accessor_holder.py(432)createAllAccessorHolderList()
    431                             getPropertySheetValueList(site,
--> 432                                                       property_sheet_name_set))
    433 

  /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/dynamic/accessor_holder.py(340)getAccessorHolderList()
    339       if expression_context is None:
--> 340         expression_context = createExpressionContext(site)
    341 

> /srv/slapgrid/slappart6/srv/runner/project/erp5/product/ERP5Type/Utils.py(1270)createExpressionContext()
   1269     import ipdb; ipdb.set_trace()
-> 1270     if pm is None or pm.isAnonymousUser():
   1271       member = None

ipdb>

[Arnaud Fontaine]

I checked myself and could reproduce the same issue. This happens when generating accessor holder for SimpleItem Property Sheet while loading Category Tool Portal Type. Considering how basic this Property Sheet is, I don't think that it does too much as you said at the beginning. Moreover, MembershipTool is not an erp5.portal_type so I can only think of a proper way to do that and that would be when it is unpickled, with something like ERP5Type/dynamic/persistent_migration. What do you think?

[Julien Muchembled]

I want our low-level automatic migration code to be in a single place, and not scattered. Currently, everything is synchronizeDynamicModules and there does not seem to be a better place; even if there was, that would be a lot of work to stabilize.

IOW, the problem is that we don't a single place driving migration/bootstrap (synchronizeDynamicModules) and reset of dynamic classes (initially synchronizeDynamicModules, but now things are moved out of it, like this __of__ method or ComponentTool).

• On one hand, we could change ERP5Site.__of__ to force a call to synchronizeDynamicModules the first time of the process life, but that would duplicate the logic in synchronizeDynamicModules.
• On the other hand, some code checking the state of portal_membership could be added somewhere during the processing of self.portal_components (maybe even migrate there): ugly for reason I gave at the beginning and also because the check would be done several times during the process lifetime.

I'm annoyed because without care the code is going to be indubitably awful.

[Julien Muchembled]

Maybe my main issue is the introduction of a second cookie whereas the code was initially designed to be controlled only by the one on the portal:

1. a single check (of the portal cookie) at most one per transaction (use of tv)
2. if new cookie (this includes a restart of the process), check if it's actually a restart and perform any required migration
3. initialize anything, and that could be components

I wish we go back to that.

[Julien Muchembled]

Maybe my main issue is the introduction of a second cookie

I see that initially it wasn't meant to be plugged into __of__ but now...

[Arnaud Fontaine]

I agree that adding yet another place is not a good idea. However, from the beginning synchronizeDynamicModules is about reset (called many times) and AFAIK has never been about migration (called only once). It was added there because it was convenient to have it there, not because it was supposed to be there... Also, migration code is already being scattered (persistent_migration.py and synchronizeDynamicModules), even though that's not an ideal situation. Considering that migration code is often being added, maybe it is time to think about a better place for automatic migration code.

About the second cookie, initially I wanted to have a single cookie but it was not feasible (I think there was another reason besides of the obvious one which is that we may want to reset Portal Types and not Components).

[Julien Muchembled]

It was added there because it was convenient to have it there, not because it was supposed to be there...

Not wrong but not pertinent. We just needed a place where to execute things as soon as possible when a site is first accessed. And here, there was a way to do it efficiently.

It was just about optimizing to common case, i.e. make ERP5Site.__of__ do as little as possible. 2 things to check: even if they're different things, it's so easy to group them in a single check for the common case that I don't see why we would do differently.

Testing migration code is nightmare because it's hard to set up a DB in an old state and even more to do it automatically. I don't want to touch the migration code for this reason. It's migration code anyway: we don't care if it's ugly inside.

migration code is already being scattered

Not really. We have the core part in synchronizeDynamicModules, everything else that plug into it, for any complex migration step to perform. Not the kind of scattering I'm talking about.

About the second cookie, initially I wanted to have a single cookie but it was not feasible (I think there was another reason besides of the obvious one which is that we may want to reset Portal Types and not Components).

I don't deny the need to reset only a part of dynamic things. I'd like the portal cookie to become a master one, i.e. "Is there anything to reset?", which is something that rarely happens.

Meanwhile, we could try to reverse things by checking the portal cookie first. Make synchronizeDynamicModules the core function for deciding what to reset, and call methods on portal_components from there.