[storage] use mkstemp to create files in bfss
Avoids race condition, at the cost of slightly less predictable file names.
--- 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: