[storage] use mkstemp to create files in bfss
authorJulien Cristau <julien.cristau@logilab.fr>
Mon, 23 Jun 2014 16:54:35 +0200
changeset 10453 76f601a90aa7
parent 10452 f3dec653a27d
child 10454 20f45a9b385c
[storage] use mkstemp to create files in bfss Avoids race condition, at the cost of slightly less predictable file names.
server/sources/storages.py
server/test/unittest_storage.py
--- a/server/sources/storages.py	Thu Jun 11 09:51:30 2015 +0200
+++ b/server/sources/storages.py	Mon Jun 23 16:54:35 2014 +0200
@@ -21,6 +21,7 @@
 import sys
 from os import unlink, path as osp
 from contextlib import contextmanager
+import tempfile
 
 from yams.schema import role_name
 
@@ -84,20 +85,11 @@
 # * handle backup/restore
 
 def uniquify_path(dirpath, basename):
-    """return a unique file name for `basename` in `dirpath`, or None
-    if all attemps failed.
-
-    XXX subject to race condition.
+    """return a file descriptor and unique file name for `basename` in `dirpath`
     """
-    path = osp.join(dirpath, basename.replace(osp.sep, '-'))
-    if not osp.isfile(path):
-        return path
+    path = basename.replace(osp.sep, '-')
     base, ext = osp.splitext(path)
-    for i in xrange(1, 256):
-        path = '%s%s%s' % (base, i, ext)
-        if not osp.isfile(path):
-            return path
-    return None
+    return tempfile.mkstemp(prefix=base, suffix=ext, dir=dirpath)
 
 @contextmanager
 def fsimport(session):
@@ -122,16 +114,13 @@
         # 0444 as in "only allow read bit in permission"
         self._wmode = wmode
 
-    def _writecontent(self, path, binary):
+    def _writecontent(self, fd, binary):
         """write the content of a binary in readonly file
 
-        As the bfss never alter a create file it does not prevent it to work as
-        intended. This is a beter safe than sorry approach.
+        As the bfss never alters an existing file it does not prevent it from
+        working as intended. This is a better safe than sorry approach.
         """
-        write_flag = os.O_WRONLY | os.O_CREAT | os.O_EXCL
-        if sys.platform == 'win32':
-            write_flag |= os.O_BINARY
-        fd = os.open(path, write_flag, self._wmode)
+        os.fchmod(fd, self._wmode)
         fileobj = os.fdopen(fd, 'wb')
         binary.to_file(fileobj)
         fileobj.close()
@@ -155,10 +144,10 @@
             entity._cw_dont_cache_attribute(attr, repo_side=True)
         else:
             binary = entity.cw_edited.pop(attr)
-            fpath = self.new_fs_path(entity, attr)
+            fd, fpath = self.new_fs_path(entity, attr)
             # bytes storage used to store file's path
             entity.cw_edited.edited_attribute(attr, Binary(fpath))
-            self._writecontent(fpath, binary)
+            self._writecontent(fd, binary)
             AddFileOp.get_instance(entity._cw).add_data(fpath)
         return binary
 
@@ -189,10 +178,9 @@
                 fpath = None
             else:
                 # Get filename for it
-                fpath = self.new_fs_path(entity, attr)
-                assert not osp.exists(fpath)
+                fd, fpath = self.new_fs_path(entity, attr)
                 # write attribute value on disk
-                self._writecontent(fpath, binary)
+                self._writecontent(fd, binary)
                 # Mark the new file as added during the transaction.
                 # The file will be removed on rollback
                 AddFileOp.get_instance(entity._cw).add_data(fpath)
@@ -224,13 +212,13 @@
         name = entity.cw_attr_metadata(attr, 'name')
         if name is not None:
             basename.append(name.encode(self.fsencoding))
-        fspath = uniquify_path(self.default_directory,
+        fd, fspath = uniquify_path(self.default_directory,
                                '_'.join(basename))
         if fspath is None:
             msg = entity._cw._('failed to uniquify path (%s, %s)') % (
                 self.default_directory, '_'.join(basename))
             raise ValidationError(entity.eid, {role_name(attr, 'subject'): msg})
-        return fspath
+        return fd, fspath
 
     def current_fs_path(self, entity, attr):
         """return the current fs_path of the attribute, or None is the attr is
--- a/server/test/unittest_storage.py	Thu Jun 11 09:51:30 2015 +0200
+++ b/server/test/unittest_storage.py	Mon Jun 23 16:54:35 2014 +0200
@@ -20,6 +20,7 @@
 from logilab.common.testlib import unittest_main, tag, Tags
 from cubicweb.devtools.testlib import CubicWebTC
 
+from glob import glob
 import os
 import os.path as osp
 import shutil
@@ -88,16 +89,21 @@
     def test_bfss_storage(self):
         with self.admin_access.repo_cnx() as cnx:
             f1 = self.create_file(cnx)
-            expected_filepath = osp.join(self.tempdir, '%s_data_%s' %
-                                         (f1.eid, f1.data_name))
-            self.assertTrue(osp.isfile(expected_filepath))
+            filepaths = glob(osp.join(self.tempdir, '%s_data_*' % f1.eid))
+            self.assertEqual(len(filepaths), 1, filepaths)
+            expected_filepath = filepaths[0]
             # file should be read only
             self.assertFalse(os.access(expected_filepath, os.W_OK))
             self.assertEqual(file(expected_filepath).read(), 'the-data')
             cnx.rollback()
             self.assertFalse(osp.isfile(expected_filepath))
+            filepaths = glob(osp.join(self.tempdir, '%s_data_*' % f1.eid))
+            self.assertEqual(len(filepaths), 0, filepaths)
             f1 = self.create_file(cnx)
             cnx.commit()
+            filepaths = glob(osp.join(self.tempdir, '%s_data_*' % f1.eid))
+            self.assertEqual(len(filepaths), 1, filepaths)
+            expected_filepath = filepaths[0]
             self.assertEqual(file(expected_filepath).read(), 'the-data')
             f1.cw_set(data=Binary('the new data'))
             cnx.rollback()
@@ -114,7 +120,9 @@
         with self.admin_access.repo_cnx() as cnx:
             f1 = self.create_file(cnx)
             expected_filepath = osp.join(self.tempdir, '%s_data_%s' % (f1.eid, f1.data_name))
-            self.assertEqual(self.fspath(cnx, f1), expected_filepath)
+            base, ext = osp.splitext(expected_filepath)
+            self.assertTrue(self.fspath(cnx, f1).startswith(base))
+            self.assertTrue(self.fspath(cnx, f1).endswith(ext))
 
     def test_bfss_fs_importing_doesnt_touch_path(self):
         with self.admin_access.repo_cnx() as cnx: