[FIX] ir.attachment: less non-transactional side-effects during deletion

When deleting filesystem-backed attachements, the
deletion on the file-system is not transactional.
In the event of a transaction rollback, the file
deletion would not be rolled back, which is a
dangerous side-effect.

This can happen for example when several transactions
try to delete the same file(s) at the same time.
The duplicate deletions might be detected by the
database (being concurrent update errors), and rolled
back at the point of the DELETE query, to be retried.
If the files have already been deleted in the file
system it before the rollback, it leaves the system
in an inconsistent state, at least temporarily.

One case where we have seen it is when web bundles
are loaded by many web users at the same time, right
after being updated (and thus invalidated).
As they are currently cached as ir.attachment records,
this often causes a corruption of the cache.

diff --git a/openerp/addons/base/ir/ir_attachment.py b/openerp/addons/base/ir/ir_attachment.py
index c636463..d3ecfd1 100644 (file)
--- a/openerp/addons/base/ir/ir_attachment.py
+++ b/openerp/addons/base/ir/ir_attachment.py
@@ -138,9 +138,9 @@ class ir_attachment(osv.osv):
         return fname
 
     def _file_delete(self, cr, uid, fname):
-        count = self.search(cr, 1, [('store_fname','=',fname)], count=True)
+        count = self.search_count(cr, 1, [('store_fname','=',fname)])
         full_path = self._full_path(cr, uid, fname)
-        if count <= 1 and os.path.exists(full_path):
+        if not count and os.path.exists(full_path):
             try:
                 os.unlink(full_path)
             except OSError:
@@ -170,14 +170,18 @@ class ir_attachment(osv.osv):
         location = self._storage(cr, uid, context)
         file_size = len(value.decode('base64'))
         attach = self.browse(cr, uid, id, context=context)
-        if attach.store_fname:
-            self._file_delete(cr, uid, attach.store_fname)
+        fname_to_delete = attach.store_fname
         if location != 'db':
             fname = self._file_write(cr, uid, value)
             # SUPERUSER_ID as probably don't have write access, trigger during create
             super(ir_attachment, self).write(cr, SUPERUSER_ID, [id], {'store_fname': fname, 'file_size': file_size, 'db_datas': False}, context=context)
         else:
             super(ir_attachment, self).write(cr, SUPERUSER_ID, [id], {'db_datas': value, 'file_size': file_size, 'store_fname': False}, context=context)
+
+        # After de-referencing the file in the database, check whether we need
+        # to garbage-collect it on the filesystem
+        if fname_to_delete:
+            self._file_delete(cr, uid, fname_to_delete)
         return True
 
     _name = 'ir.attachment'
@@ -315,10 +319,19 @@ class ir_attachment(osv.osv):
         if isinstance(ids, (int, long)):
             ids = [ids]
         self.check(cr, uid, ids, 'unlink', context=context)
-        for attach in self.browse(cr, uid, ids, context=context):
-            if attach.store_fname:
-                self._file_delete(cr, uid, attach.store_fname)
-        return super(ir_attachment, self).unlink(cr, uid, ids, context)
+
+        # First delete in the database, *then* in the filesystem if the
+        # database allowed it. Helps avoid errors when concurrent transactions
+        # are deleting the same file, and some of the transactions are
+        # rolled back by PostgreSQL (due to concurrent updates detection).
+        to_delete = [a.store_fname
+                        for a in self.browse(cr, uid, ids, context=context)
+                            if a.store_fname]
+        res = super(ir_attachment, self).unlink(cr, uid, ids, context)
+        for file_path in to_delete:
+            self._file_delete(cr, uid, file_path)
+
+        return res
 
     def create(self, cr, uid, values, context=None):
         self.check(cr, uid, [], mode='write', context=context, values=values)