From f83d5aa89f23d64927ced85756f44d52bb0d16a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gina=20H=C3=A4u=C3=9Fge?= Date: Mon, 5 Oct 2015 18:07:43 +0200 Subject: [PATCH 1/2] Fix: Use atomic writes for all save processes That includes uploaded files, profiles, caching files, settings and user directories. --- src/octoprint/filemanager/util.py | 4 +++- src/octoprint/plugins/cura/__init__.py | 2 +- src/octoprint/plugins/pluginmanager/__init__.py | 2 +- src/octoprint/settings.py | 4 +++- src/octoprint/users.py | 4 +++- 5 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/octoprint/filemanager/util.py b/src/octoprint/filemanager/util.py index 105987f3..f3fe20fb 100644 --- a/src/octoprint/filemanager/util.py +++ b/src/octoprint/filemanager/util.py @@ -7,6 +7,8 @@ __copyright__ = "Copyright (C) 2015 The OctoPrint Project - Released under terms import io +from octoprint.util import atomic_write + class AbstractFileWrapper(object): """ Wrapper for file representations to save to storages. @@ -85,7 +87,7 @@ class StreamWrapper(AbstractFileWrapper): """ import shutil - with open(path, "wb") as dest: + with atomic_write(path, "wb") as dest: with self.stream() as source: shutil.copyfileobj(source, dest) diff --git a/src/octoprint/plugins/cura/__init__.py b/src/octoprint/plugins/cura/__init__.py index 30de8874..fd326a10 100644 --- a/src/octoprint/plugins/cura/__init__.py +++ b/src/octoprint/plugins/cura/__init__.py @@ -393,7 +393,7 @@ class CuraPlugin(octoprint.plugin.SlicerPlugin, def _save_profile(self, path, profile, allow_overwrite=True): import yaml - with open(path, "wb") as f: + with octoprint.util.atomic_write(path, "wb") as f: yaml.safe_dump(profile, f, default_flow_style=False, indent=" ", allow_unicode=True) def _convert_to_engine(self, profile_path, printer_profile, posX, posY): diff --git a/src/octoprint/plugins/pluginmanager/__init__.py b/src/octoprint/plugins/pluginmanager/__init__.py index a93b6cc2..0c40562a 100644 --- a/src/octoprint/plugins/pluginmanager/__init__.py +++ b/src/octoprint/plugins/pluginmanager/__init__.py @@ -536,7 +536,7 @@ class PluginManagerPlugin(octoprint.plugin.SimpleApiPlugin, try: import json - with open(self._repository_cache_path, "w+b") as f: + with octoprint.util.atomic_write(self._repository_cache_path, "wb") as f: json.dump(repo_data, f) except Exception as e: self._logger.exception("Error while saving repository data to {}: {}".format(self._repository_cache_path, str(e))) diff --git a/src/octoprint/settings.py b/src/octoprint/settings.py index fa3f0e5f..733d24b8 100644 --- a/src/octoprint/settings.py +++ b/src/octoprint/settings.py @@ -30,6 +30,8 @@ import logging import re import uuid +from octoprint.util import atomic_write + _APPNAME = "OctoPrint" _instance = None @@ -1057,7 +1059,7 @@ class Settings(object): path, _ = os.path.split(filename) if not os.path.exists(path): os.makedirs(path) - with open(filename, "w+") as f: + with atomic_write(filename, "wb") as f: f.write(script) def _default_basedir(applicationName): diff --git a/src/octoprint/users.py b/src/octoprint/users.py index 76bf23bd..d4890a64 100644 --- a/src/octoprint/users.py +++ b/src/octoprint/users.py @@ -17,6 +17,8 @@ import logging from octoprint.settings import settings +from octoprint.util import atomic_write + class UserManager(object): valid_roles = ["user", "admin"] @@ -217,7 +219,7 @@ class FilebasedUserManager(UserManager): "settings": user._settings } - with open(self._userfile, "wb") as f: + with atomic_write(self._userfile, "wb") as f: yaml.safe_dump(data, f, default_flow_style=False, indent=" ", allow_unicode=True) self._dirty = False self._load() From 85e6ae8e8278721affae4524c9827d63de8d8986 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gina=20H=C3=A4u=C3=9Fge?= Date: Wed, 7 Oct 2015 17:45:07 +0200 Subject: [PATCH 2/2] Fixed a unit test that broke by switching to atomic_write --- tests/filemanager/test_filemanager.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/filemanager/test_filemanager.py b/tests/filemanager/test_filemanager.py index 110df373..1da5f39f 100644 --- a/tests/filemanager/test_filemanager.py +++ b/tests/filemanager/test_filemanager.py @@ -109,13 +109,13 @@ class FileManagerTest(unittest.TestCase): self.assertEquals(metadata, expected) self.local_storage.get_metadata.assert_called_once_with("test.file") - @mock.patch("__builtin__.open", new_callable=mock.mock_open) + @mock.patch("octoprint.filemanager.util.atomic_write") @mock.patch("io.FileIO") @mock.patch("shutil.copyfileobj") @mock.patch("os.remove") @mock.patch("tempfile.NamedTemporaryFile") @mock.patch("time.time", side_effect=[1411979916.422, 1411979932.116]) - def test_slice(self, mocked_time, mocked_tempfile, mocked_os, mocked_shutil, mocked_fileio, mocked_open): + def test_slice(self, mocked_time, mocked_tempfile, mocked_os, mocked_shutil, mocked_fileio, mocked_atomic_write): callback = mock.MagicMock() callback_args = ("one", "two", "three") @@ -187,8 +187,8 @@ class FileManagerTest(unittest.TestCase): self.local_storage.add_file.assert_called_once_with("dest.file", mock.ANY, printer_profile=expected_printer_profile, allow_overwrite=True, links=expected_links) # assert that the generated gcode was manipulated as required - expected_open_calls = [mock.call("prefix/dest.file", "wb")] - self.assertEquals(mocked_open.call_args_list, expected_open_calls) + expected_atomic_write_calls = [mock.call("prefix/dest.file", "wb")] + self.assertEquals(mocked_atomic_write.call_args_list, expected_atomic_write_calls) #mocked_open.return_value.write.assert_called_once_with(";Generated from source.file aabbccddeeff\r") # assert that shutil was asked to copy the concatenated multistream