Merge pull request #468 from Unrud/disablefsync

Add option to disable syncing to disk

config

@@ -103,6 +103,11 @@
 # Folder for storing local collections, created if not present
 #filesystem_folder = ~/.config/radicale/collections
 
+# Sync all changes to disk during requests. (This can impair performance.)
+# Disabling it increases the risk of data loss, when the system crashes or
+# power fails!
+#fsync = True
+
 # Command that is run after changes to storage
 #hook =
 # Example: git add -A && (git diff --cached --quiet || git commit -m "Changes by "%(user)s)

radicale/config.py

@@ -58,6 +58,7 @@ INITIAL_CONFIG = {
         "type": "multifilesystem",
         "filesystem_folder": os.path.expanduser(
             "~/.config/radicale/collections"),
+        "fsync": "True",
         "hook": ""},
     "logging": {
         "config": "/etc/radicale/logging",

radicale/storage.py

@@ -37,9 +37,8 @@ from hashlib import md5
 from importlib import import_module
 from itertools import groupby
 from random import getrandbits
-from tempfile import TemporaryDirectory
+from tempfile import TemporaryDirectory, NamedTemporaryFile
 
-from atomicwrites import AtomicWriter
 import vobject
 
 
@@ -168,23 +167,6 @@ def path_to_filesystem(root, *paths):
     return safe_path
 
 
-def sync_directory(path):
-    """Sync directory to disk.
-
-    This only works on POSIX and does nothing on other systems.
-
-    """
-    if os.name == "posix":
-        fd = os.open(path, 0)
-        try:
-            if hasattr(fcntl, "F_FULLFSYNC"):
-                fcntl.fcntl(fd, fcntl.F_FULLFSYNC)
-            else:
-                os.fsync(fd)
-        finally:
-            os.close(fd)
-
-
 class UnsafePathError(ValueError):
     def __init__(self, path):
         message = "Can't translate name safely to filesystem: %s" % path
@@ -209,16 +191,6 @@ class EtagMismatchError(ValueError):
         super().__init__(message)
 
 
-class _EncodedAtomicWriter(AtomicWriter):
-    def __init__(self, path, encoding, mode="w", overwrite=True):
-        self._encoding = encoding
-        return super().__init__(path, mode, overwrite=True)
-
-    def get_fileobject(self, **kwargs):
-        return super().get_fileobject(
-            encoding=self._encoding, prefix=".Radicale.tmp-", **kwargs)
-
-
 class Item:
     def __init__(self, collection, item, href, last_modified=None):
         self.collection = collection
@@ -420,8 +392,23 @@ class Collection(BaseCollection):
 
     @contextmanager
     def _atomic_write(self, path, mode="w"):
-        with _EncodedAtomicWriter(path, self.encoding, mode).open() as fd:
-            yield fd
+        dir = os.path.dirname(path)
+        tmp = NamedTemporaryFile(mode=mode, dir=dir, encoding=self.encoding,
+                                 delete=False, prefix=".Radicale.tmp-")
+        try:
+            yield tmp
+            if self.configuration.getboolean("storage", "fsync"):
+                if os.name == "posix" and hasattr(fcntl, "F_FULLFSYNC"):
+                    fcntl.fcntl(tmp.fileno(), fcntl.F_FULLFSYNC)
+                else:
+                    os.fsync(tmp.fileno())
+            tmp.close()
+            os.rename(tmp.name, path)
+        except:
+            tmp.close()
+            os.remove(tmp.name)
+            raise
+        self._sync_directory(dir)
 
     def _find_available_file_name(self):
         # Prevent infinite loop
@@ -431,6 +418,25 @@ class Collection(BaseCollection):
                 return file_name
         raise FileExistsError(errno.EEXIST, "No usable file name found")
 
+    @classmethod
+    def _sync_directory(cls, path):
+        """Sync directory to disk.
+
+        This only works on POSIX and does nothing on other systems.
+
+        """
+        if not cls.configuration.getboolean("storage", "fsync"):
+            return
+        if os.name == "posix":
+            fd = os.open(path, 0)
+            try:
+                if hasattr(fcntl, "F_FULLFSYNC"):
+                    fcntl.fcntl(fd, fcntl.F_FULLFSYNC)
+                else:
+                    os.fsync(fd)
+            finally:
+                os.close(fd)
+
     @classmethod
     def _makedirs_synced(cls, filesystem_path):
         """Recursively create a directory and its parents in a sync'ed way.
@@ -447,7 +453,7 @@ class Collection(BaseCollection):
             cls._makedirs_synced(parent_filesystem_path)
         # Possible race!
         os.makedirs(filesystem_path, exist_ok=True)
-        sync_directory(parent_filesystem_path)
+        cls._sync_directory(parent_filesystem_path)
 
     @classmethod
     def discover(cls, path, depth="0"):
@@ -561,7 +567,7 @@ class Collection(BaseCollection):
             if os.path.exists(filesystem_path):
                 os.rename(filesystem_path, os.path.join(tmp_dir, "delete"))
             os.rename(tmp_filesystem_path, filesystem_path)
-            sync_directory(parent_dir)
+            cls._sync_directory(parent_dir)
 
         return cls(sane_path, principal=principal)
 
@@ -570,9 +576,9 @@ class Collection(BaseCollection):
         os.rename(
             path_to_filesystem(item.collection._filesystem_path, item.href),
             path_to_filesystem(to_collection._filesystem_path, to_href))
-        sync_directory(to_collection._filesystem_path)
+        cls._sync_directory(to_collection._filesystem_path)
         if item.collection._filesystem_path != to_collection._filesystem_path:
-            sync_directory(item.collection._filesystem_path)
+            cls._sync_directory(item.collection._filesystem_path)
 
     def list(self):
         try:
@@ -648,9 +654,9 @@ class Collection(BaseCollection):
                                             dir=parent_dir) as tmp_dir:
                         os.rename(self._filesystem_path, os.path.join(
                             tmp_dir, os.path.basename(self._filesystem_path)))
-                        sync_directory(parent_dir)
+                        self._sync_directory(parent_dir)
                 else:
-                    sync_directory(parent_dir)
+                    self._sync_directory(parent_dir)
         else:
             # Delete an item
             if not is_safe_filesystem_path_component(href):
@@ -663,7 +669,7 @@ class Collection(BaseCollection):
             if etag and etag != get_etag(text):
                 raise EtagMismatchError(etag, get_etag(text))
             os.remove(path)
-            sync_directory(os.path.dirname(path))
+            self._sync_directory(os.path.dirname(path))
 
     def get_meta(self, key):
         if os.path.exists(self._props_path):

radicale/tests/test_base.py

@@ -726,6 +726,8 @@ class TestMultiFileSystem(BaseRequests, BaseTest):
         super().setup()
         self.colpath = tempfile.mkdtemp()
         self.configuration.set("storage", "filesystem_folder", self.colpath)
+        # Disable syncing to disk for better performance
+        self.configuration.set("storage", "fsync", "False")
         self.application = Application(self.configuration, self.logger)
 
     def teardown(self):
@@ -740,6 +742,8 @@ class TestCustomStorageSystem(BaseRequests, BaseTest):
         super().setup()
         self.colpath = tempfile.mkdtemp()
         self.configuration.set("storage", "filesystem_folder", self.colpath)
+        # Disable syncing to disk for better performance
+        self.configuration.set("storage", "fsync", "False")
         self.application = Application(self.configuration, self.logger)
 
     def teardown(self):

setup.py

@@ -66,7 +66,7 @@ setup(
     packages=["radicale"],
     provides=["radicale"],
     scripts=["bin/radicale"],
-    install_requires=["vobject", "atomicwrites"],
+    install_requires=["vobject"],
     setup_requires=pytest_runner,
     tests_require=[
         "pytest-runner", "pytest-cov", "pytest-flake8", "pytest-isort"],