[REF] registry: always perform registry/cache signaling at the end of request

Problem: the update of custom models/fields is not fully transactional, and may
potentially lead to an inconsistent database.  An other problem is creating two
custom fields by writing on a model: if the second one fails, the first one has
been committed without notice.  Retrying the request will give an unexpected
error (duplicate field name).

Solution: never commit in the middle of a request.  If the changes have an
impact on the registry, then mark it as invalid (with a new flag), and signal
registry invalidation after everything has been committed.  If the request
fails, reset the registry.  Both registry and cache invalidation are handled
the same way.

addons/base_automation/models/base_automation.py

@@ -114,7 +114,7 @@ class BaseAutomation(models.Model):
             self._cr.commit()
             self.env.reset()
             registry = Registry.new(self._cr.dbname)
-            registry.signal_registry_change()
+            registry.registry_invalidated = True
 
     def _get_actions(self, records, triggers):
         """ Return the actions of the given triggers for records' model. The

addons/mail/models/ir_model.py

@@ -27,7 +27,6 @@ class IrModel(models.Model):
             # update database schema of models
             models = self.pool.descendants(self.mapped('model'), '_inherits')
             self.pool.init_models(self._cr, models, dict(self._context, update_custom_fields=True))
-            self.pool.signal_registry_change()
         else:
             res = super(IrModel, self).write(vals)
         return res

odoo/addons/base/ir/ir_cron.py

@@ -95,8 +95,9 @@ class ir_cron(models.Model):
             if start_time and _logger.isEnabledFor(logging.DEBUG):
                 end_time = time.time()
                 _logger.debug('%.3fs (cron %s, server action %d with uid %d)', end_time - start_time, cron_name, server_action_id, self.env.uid)
-            self.pool.signal_caches_change()
+            self.pool.signal_changes()
         except Exception as e:
+            self.pool.reset_changes()
             self._handle_callback_exception(cron_name, server_action_id, job_id, e)
 
     @classmethod

odoo/addons/base/ir/ir_model.py

@@ -180,10 +180,8 @@ class IrModel(models.Model):
         # Reload registry for normal unlink only. For module uninstall, the
         # reload is done independently in odoo.modules.loading.
         if not self._context.get(MODULE_UNINSTALL_FLAG):
-            self._cr.commit()  # must be committed before reloading registry in new cursor
-            api.Environment.reset()
-            registry = Registry.new(self._cr.dbname)
-            registry.signal_registry_change()
+            # setup models; this automatically removes model from registry
+            self.pool.setup_models(self._cr)
 
         return res
 
@@ -211,7 +209,6 @@ class IrModel(models.Model):
             self.pool.setup_models(self._cr)
             # update database schema
             self.pool.init_models(self._cr, [vals['model']], dict(self._context, update_custom_fields=True))
-            self.pool.signal_registry_change()
         return res
 
     @api.model
@@ -275,6 +272,11 @@ class IrModel(models.Model):
 
     def _add_manual_models(self):
         """ Add extra models to the registry. """
+        # clean up registry first
+        for name, model_class in self.pool.items():
+            if model_class._custom:
+                del self.pool.models[name]
+        # add manual models
         cr = self.env.cr
         cr.execute('SELECT * FROM ir_model WHERE state=%s', ['manual'])
         for model_data in cr.dictfetchall():
@@ -591,12 +593,11 @@ class IrModelFields(models.Model):
         # The field we just deleted might be inherited, and the registry is
         # inconsistent in this case; therefore we reload the registry.
         if not self._context.get(MODULE_UNINSTALL_FLAG):
-            self._cr.commit()
-            api.Environment.reset()
-            registry = Registry.new(self._cr.dbname)
-            models = registry.descendants(model_names, '_inherits')
-            registry.init_models(self._cr, models, dict(self._context, update_custom_fields=True))
-            registry.signal_registry_change()
+            # setup models; this re-initializes models in registry
+            self.pool.setup_models(self._cr)
+            # update database schema of model and its descendant models
+            models = self.pool.descendants(model_names, '_inherits')
+            self.pool.init_models(self._cr, models, dict(self._context, update_custom_fields=True))
 
         return res
 
@@ -628,7 +629,6 @@ class IrModelFields(models.Model):
                 # update database schema of model and its descendant models
                 models = self.pool.descendants([vals['model']], '_inherits')
                 self.pool.init_models(self._cr, models, dict(self._context, update_custom_fields=True))
-                self.pool.signal_registry_change()
 
         return res
 
@@ -698,9 +698,6 @@ class IrModelFields(models.Model):
             models = self.pool.descendants(patched_models, '_inherits')
             self.pool.init_models(self._cr, models, dict(self._context, update_custom_fields=True))
 
-        if column_rename or patched_models:
-            self.pool.signal_registry_change()
-
         return res
 
     @api.multi

odoo/addons/base/res/res_font.py

@@ -91,7 +91,7 @@ class ResFont(models.Model):
         # Remove inexistent fonts
         self.search([('name', 'not in', existing_font_names), ('path', '!=', '/dev/null')]).unlink()
 
-        self.pool.signal_caches_change()
+        self.pool.cache_invalidated = True
         return self._sync()
 
     def _sync(self):

odoo/addons/base/tests/test_ir_actions.py

@@ -188,22 +188,19 @@ class TestCustomFields(common.TransactionCase):
     MODEL = 'res.partner'
 
     def setUp(self):
-        # use a test cursor instead of a real cursor
+        # check that the registry is properly reset
         registry = odoo.registry()
-        registry.enter_test_mode()
         fnames = set(registry[self.MODEL]._fields)
-
         @self.addCleanup
-        def callback():
-            registry.leave_test_mode()
-            # the tests may have modified the registry, reset it
-            with registry.cursor() as cr:
-                registry.clear_caches()
-                registry.setup_models(cr)
-                assert set(registry[self.MODEL]._fields) == fnames
+        def check_registry():
+            assert set(registry[self.MODEL]._fields) == fnames
 
         super(TestCustomFields, self).setUp()
 
+        # use a test cursor instead of a real cursor
+        self.registry.enter_test_mode()
+        self.addCleanup(self.registry.leave_test_mode)
+
         # do not reload the registry after removing a field
         self.env = self.env(context={'_force_unlink': True})
 

odoo/http.py

@@ -278,6 +278,9 @@ class WebRequest(object):
         if self._cr:
             if exc_type is None and not self._failed:
                 self._cr.commit()
+                self.registry.signal_changes()
+            else:
+                self.registry.reset_changes()
             self._cr.close()
         # just to be sure no one tries to re-use the request
         self.disable_db = True
@@ -1467,7 +1470,6 @@ class Root(object):
                             result = _dispatch_nodb()
                     else:
                         result = ir_http._dispatch()
-                        ir_http.pool.signal_caches_change()
                 else:
                     result = _dispatch_nodb()
 

odoo/models.py

@@ -1541,11 +1541,7 @@ class BaseModel(object):
         This clears the caches associated to methods decorated with
         ``tools.ormcache`` or ``tools.ormcache_multi``.
         """
-        try:
-            cls.pool.cache.clear()
-            cls.pool.cache_cleared = True
-        except AttributeError:
-            pass
+        cls.pool._clear_cache()
 
     @api.model
     def _read_group_fill_results(self, domain, groupby, remaining_groupbys,

odoo/modules/loading.py

@@ -133,6 +133,7 @@ def load_module_graph(cr, graph, status=None, perform_checks=True, skip_modules=
         if hasattr(package, 'init') or hasattr(package, 'update') or package.state in ('to install', 'to upgrade'):
             registry.setup_models(cr)
             registry.init_models(cr, model_names, {'module': package.name})
+            cr.commit()
 
         idref = {}
 

odoo/modules/registry.py

@@ -5,7 +5,7 @@
 
 """
 from collections import Mapping, defaultdict, deque
-from contextlib import closing
+from contextlib import closing, contextmanager
 from functools import partial
 from operator import attrgetter
 from weakref import WeakValueDictionary
@@ -98,10 +98,8 @@ class Registry(Mapping):
                     cr.commit()
 
         registry.ready = True
+        registry.registry_invalidated = bool(update_module)
 
-        if update_module:
-            # only in case of update, otherwise we'll have an infinite reload loop!
-            registry.signal_registry_change()
         return registry
 
     def init(self, db_name):
@@ -135,10 +133,9 @@ class Registry(Mapping):
         self.registry_sequence = None
         self.cache_sequence = None
 
-        self.cache = LRU(8192)
-        # Flag indicating if at least one model cache has been cleared.
-        # Useful only in a multi-process context.
-        self.cache_cleared = False
+        # Flags indicating invalidation of the registry or the cache.
+        self.registry_invalidated = False
+        self.cache_invalidated = False
 
         with closing(self.cursor()) as cr:
             has_unaccent = odoo.modules.db.has_unaccent(cr)
@@ -151,8 +148,9 @@ class Registry(Mapping):
         """ Delete the registry linked to a given database. """
         with cls._lock:
             if db_name in cls.registries:
-                cls.registries[db_name].clear_caches()
-                del cls.registries[db_name]
+                registry = cls.registries.pop(db_name)
+                registry.clear_caches()
+                registry.registry_invalidated = True
 
     @classmethod
     def delete_all(cls):
@@ -278,6 +276,8 @@ class Registry(Mapping):
         for model in models:
             model._setup_complete()
 
+        self.registry_invalidated = True
+
     def post_init(self, func, *args, **kwargs):
         """ Register a function to call at the end of :meth:`~.init_models`. """
         self._post_init_queue.append(partial(func, *args, **kwargs))
@@ -307,7 +307,6 @@ class Registry(Mapping):
 
         if models:
             models[0].recompute()
-        cr.commit()
 
         # make sure all tables are present
         missing = [name
@@ -321,17 +320,26 @@ class Registry(Mapping):
                 if name in missing:
                     _logger.info("Recreate table of model %s.", name)
                     env[name].init()
-            cr.commit()
             # check again, and log errors if tables are still missing
             for name, model in env.items():
                 if not model._abstract and not table_exists(cr, model._table):
                     _logger.error("Model %s has no table.", name)
 
+    @lazy_property
+    def cache(self):
+        """ A cache for model methods. """
+        # this lazy_property is automatically reset by lazy_property.reset_all()
+        return LRU(8192)
+
+    def _clear_cache(self):
+        """ Clear the cache and mark it as invalidated. """
+        self.cache.clear()
+        self.cache_invalidated = True
+
     def clear_caches(self):
         """ Clear the caches associated to methods decorated with
         ``tools.ormcache`` or ``tools.ormcache_multi`` for all the models.
         """
-        self.cache.clear()
         for model in self.models.itervalues():
             model.clear_caches()
 
@@ -381,29 +389,50 @@ class Registry(Mapping):
             elif self.cache_sequence != c:
                 _logger.info("Invalidating all model caches after database signaling.")
                 self.clear_caches()
-                self.cache_cleared = False
+                self.cache_invalidated = False
             self.registry_sequence = r
             self.cache_sequence = c
 
         return self
 
-    def signal_registry_change(self):
-        """ Notifies other processes that the registry has changed. """
-        if odoo.multi_process:
+    def signal_changes(self):
+        """ Notifies other processes if registry or cache has been invalidated. """
+        if odoo.multi_process and self.registry_invalidated:
             _logger.info("Registry changed, signaling through the database")
             with closing(self.cursor()) as cr:
                 cr.execute("select nextval('base_registry_signaling')")
                 self.registry_sequence = cr.fetchone()[0]
 
-    def signal_caches_change(self):
-        """ Notifies other processes if caches have been invalidated. """
-        if odoo.multi_process and self.cache_cleared:
-            # signal it through the database to other processes
+        # no need to notify cache invalidation in case of registry invalidation,
+        # because reloading the registry implies starting with an empty cache
+        elif odoo.multi_process and self.cache_invalidated:
             _logger.info("At least one model cache has been invalidated, signaling through the database.")
             with closing(self.cursor()) as cr:
                 cr.execute("select nextval('base_cache_signaling')")
                 self.cache_sequence = cr.fetchone()[0]
-                self.cache_cleared = False
+
+        self.registry_invalidated = False
+        self.cache_invalidated = False
+
+    def reset_changes(self):
+        """ Reset the registry and cancel all invalidations. """
+        if self.registry_invalidated:
+            with closing(self.cursor()) as cr:
+                self.setup_models(cr)
+                self.registry_invalidated = False
+        if self.cache_invalidated:
+            self.cache.clear()
+            self.cache_invalidated = False
+
+    @contextmanager
+    def manage_changes(self):
+        """ Context manager to signal/discard registry and cache invalidations. """
+        try:
+            yield self
+            self.signal_changes()
+        except Exception:
+            self.reset_changes()
+            raise
 
     def in_test_mode(self):
         """ Test whether the registry is in 'test' mode. """

odoo/service/model.py

@@ -36,8 +36,8 @@ def dispatch(method, params):
     security.check(db,uid,passwd)
     registry = odoo.registry(db).check_signaling()
     fn = globals()[method]
-    res = fn(db, uid, *params)
-    registry.signal_caches_change()
+    with registry.manage_changes():
+        res = fn(db, uid, *params)
     return res
 
 def check(f):

odoo/service/report.py

@@ -32,8 +32,8 @@ def dispatch(method, params):
     security.check(db,uid,passwd)
     registry = odoo.registry(db).check_signaling()
     fn = globals()['exp_' + method]
-    res = fn(db, uid, *params)
-    registry.signal_caches_change()
+    with registry.manage_changes():
+        res = fn(db, uid, *params)
     return res
 
 def exp_render_report(db, uid, object, ids, datas=None, context=None):

odoo/tests/common.py

@@ -156,6 +156,7 @@ class TransactionCase(BaseCase):
         def reset():
             # rollback and close the cursor, and reset the environments
             self.registry.clear_caches()
+            self.registry.reset_changes()
             self.env.reset()
             self.cr.rollback()
             self.cr.close()

odoo/tools/cache.py

@@ -92,9 +92,7 @@ class ormcache(object):
 
     def clear(self, model, *args):
         """ Clear the registry cache """
-        d, key0, _ = self.lru(model)
-        d.clear()
-        model.pool.cache_cleared = True
+        model.pool._clear_cache()
 
 
 class ormcache_context(ormcache):

odoo/tools/sql.py

@@ -189,7 +189,6 @@ def drop_index(cr, indexname, tablename):
 
 def drop_view_if_exists(cr, viewname):
     cr.execute("DROP view IF EXISTS %s CASCADE" % (viewname,))
-    cr.commit()
 
 def escape_psql(to_escape):
     return to_escape.replace('\\', r'\\').replace('%', '\%').replace('_', '\_')