[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.

15 files changed, 85 insertions, 65 deletions

diff --git a/addons/base_automation/models/base_automation.py b/addons/base_automation/models/base_automation.py
index af8363b92fa..c9c0a150364 100644
--- a/addons/base_automation/models/base_automation.py
+++ b/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
diff --git a/addons/mail/models/ir_model.py b/addons/mail/models/ir_model.py
index af2666d4fb0..e2231e38f5d 100644
--- a/addons/mail/models/ir_model.py
+++ b/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
diff --git a/odoo/addons/base/ir/ir_cron.py b/odoo/addons/base/ir/ir_cron.py
index 0c61f230813..5b0c6567ec7 100644
--- a/odoo/addons/base/ir/ir_cron.py
+++ b/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
diff --git a/odoo/addons/base/ir/ir_model.py b/odoo/addons/base/ir/ir_model.py
index f2d41c46847..49989a2c7a8 100644
--- a/odoo/addons/base/ir/ir_model.py
+++ b/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
diff --git a/odoo/addons/base/res/res_font.py b/odoo/addons/base/res/res_font.py
index 873bea5aa03..ab3ad251c6c 100644
--- a/odoo/addons/base/res/res_font.py
+++ b/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):
diff --git a/odoo/addons/base/tests/test_ir_actions.py b/odoo/addons/base/tests/test_ir_actions.py
index 344829c8181..4e0ee08d408 100644
--- a/odoo/addons/base/tests/test_ir_actions.py
+++ b/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})
 
diff --git a/odoo/http.py b/odoo/http.py
index 210361fce1b..0b3ad105e39 100644
--- a/odoo/http.py
+++ b/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()
 
diff --git a/odoo/models.py b/odoo/models.py
index 10e962531f5..4f85dbf11ac 100644
--- a/odoo/models.py
+++ b/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,
diff --git a/odoo/modules/loading.py b/odoo/modules/loading.py
index 7536c344528..f8c65d40177 100644
--- a/odoo/modules/loading.py
+++ b/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 = {}
 
diff --git a/odoo/modules/registry.py b/odoo/modules/registry.py
index 9642219a88c..e0cfb532fd1 100644
--- a/odoo/modules/registry.py
+++ b/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. """
diff --git a/odoo/service/model.py b/odoo/service/model.py
index 9ceab82b452..ba243bf432c 100644
--- a/odoo/service/model.py
+++ b/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):
diff --git a/odoo/service/report.py b/odoo/service/report.py
index faac5521b99..8229a2396e4 100644
--- a/odoo/service/report.py
+++ b/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):
diff --git a/odoo/tests/common.py b/odoo/tests/common.py
index ce7be91ffd4..78cfbfbdbf9 100644
--- a/odoo/tests/common.py
+++ b/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()
diff --git a/odoo/tools/cache.py b/odoo/tools/cache.py
index 3703fc83ab1..48dc42ba04c 100644
--- a/odoo/tools/cache.py
+++ b/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):
diff --git a/odoo/tools/sql.py b/odoo/tools/sql.py
index c4e9a143a8e..563ed08c679 100644
--- a/odoo/tools/sql.py
+++ b/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('_', '\_')