From a03dfefda9b4ea3554b9dc23ca9ebeb706b15ff1 Mon Sep 17 00:00:00 2001 From: Olivier Dony Date: Wed, 3 Sep 2014 18:39:25 +0200 Subject: [PATCH] [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. --- openerp/addons/base/ir/ir_attachment.py | 29 ++++++++++++++++++------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/openerp/addons/base/ir/ir_attachment.py b/openerp/addons/base/ir/ir_attachment.py index c636463be10..d3ecfd1b2c7 100644 --- 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)