diff --git a/notebook/services/contents/fileio.py b/notebook/services/contents/fileio.py index 2fcda7d4a..9dd283572 100644 --- a/notebook/services/contents/fileio.py +++ b/notebook/services/contents/fileio.py @@ -22,6 +22,8 @@ import nbformat from ipython_genutils.py3compat import str_to_unicode +from traitlets.config import Configurable +from traitlets import Bool def copy2_safe(src, dst, log=None): """copy src to dst @@ -39,24 +41,24 @@ def copy2_safe(src, dst, log=None): @contextmanager def atomic_writing(path, text=True, encoding='utf-8', log=None, **kwargs): """Context manager to write to a file only if the entire write is successful. - + This works by copying the previous file contents to a temporary file in the same directory, and renaming that file back to the target if the context exits with an error. If the context is successful, the new data is synced to disk and the temporary file is removed. - + Parameters ---------- path : str The target file to write to. - + text : bool, optional Whether to open the file in text mode (i.e. to write unicode). Default is True. - + encoding : str, optional The encoding to use for files opened in text mode. Default is UTF-8. - + **kwargs Passed to :func:`io.open`. """ @@ -100,7 +102,52 @@ def atomic_writing(path, text=True, encoding='utf-8', log=None, **kwargs): os.remove(tmp_path) -class FileManagerMixin(object): + +@contextmanager +def _simple_writing(path, text=True, encoding='utf-8', log=None, **kwargs): + """Context manager to write file without doing atomic writing + ( for weird filesystem eg: nfs). + + Parameters + ---------- + path : str + The target file to write to. + + text : bool, optional + Whether to open the file in text mode (i.e. to write unicode). Default is + True. + + encoding : str, optional + The encoding to use for files opened in text mode. Default is UTF-8. + + **kwargs + Passed to :func:`io.open`. + """ + # realpath doesn't work on Windows: http://bugs.python.org/issue9949 + # Luckily, we only need to resolve the file itself being a symlink, not + # any of its directories, so this will suffice: + if os.path.islink(path): + path = os.path.join(os.path.dirname(path), os.readlink(path)) + + if text: + # Make sure that text files have Unix linefeeds by default + kwargs.setdefault('newline', '\n') + fileobj = io.open(path, 'w', encoding=encoding, **kwargs) + else: + fileobj = io.open(path, 'wb', **kwargs) + + try: + yield fileobj + except: + fileobj.close() + raise + + fileobj.close() + + + + +class FileManagerMixin(Configurable): """ Mixin for ContentsAPI classes that interact with the filesystem. @@ -119,6 +166,11 @@ class FileManagerMixin(object): log : logging.Logger """ + use_atomic_writing = Bool(True, config=True, help= + """By default notebooks are saved on disk on a temporary file and then if succefully written, it replaces the old ones. + This procedure, namely 'atomic_writing', causes some bugs on file system whitout operation order enforcement (like some networked fs). + If set to False, the new notebook is written directly on the old one which could fail (eg: full filesystem or quota )""") + @contextmanager def open(self, os_path, *args, **kwargs): """wrapper around io.open that turns permission errors into 403""" @@ -128,10 +180,16 @@ class FileManagerMixin(object): @contextmanager def atomic_writing(self, os_path, *args, **kwargs): - """wrapper around atomic_writing that turns permission errors to 403""" + """wrapper around atomic_writing that turns permission errors to 403. + Depending on flag 'use_atomic_writing', the wrapper perform an actual atomic writing or + simply writes the file (whatever an old exists or not)""" with self.perm_to_403(os_path): - with atomic_writing(os_path, *args, log=self.log, **kwargs) as f: - yield f + if self.use_atomic_writing: + with atomic_writing(os_path, *args, log=self.log, **kwargs) as f: + yield f + else: + with _simple_writing(os_path, *args, log=self.log, **kwargs) as f: + yield f @contextmanager def perm_to_403(self, os_path=''): diff --git a/notebook/services/contents/tests/test_manager.py b/notebook/services/contents/tests/test_manager.py index 326280219..c13e60fa7 100644 --- a/notebook/services/contents/tests/test_manager.py +++ b/notebook/services/contents/tests/test_manager.py @@ -47,7 +47,7 @@ class TestFileContentsManager(TestCase): def symlink(self, contents_manager, src, dst): """Make a symlink to src from dst - + src and dst are api_paths """ src_os_path = contents_manager._get_os_path(src) @@ -107,7 +107,7 @@ class TestFileContentsManager(TestCase): self.assertNotEqual(cp_dir, cp_subdir) self.assertEqual(cp_dir, os.path.join(root, cpm.checkpoint_dir, cp_name)) self.assertEqual(cp_subdir, os.path.join(root, subd, cpm.checkpoint_dir, cp_name)) - + @dec.skip_win32 def test_bad_symlink(self): with TemporaryDirectory() as td: @@ -121,7 +121,7 @@ class TestFileContentsManager(TestCase): self.symlink(cm, "target", '%s/%s' % (path, 'bad symlink')) model = cm.get(path) self.assertEqual(model['content'], [file_model]) - + @dec.skip_win32 def test_good_symlink(self): with TemporaryDirectory() as td: @@ -141,19 +141,19 @@ class TestFileContentsManager(TestCase): sorted(dir_model['content'], key=lambda x: x['name']), [symlink_model, file_model], ) - + def test_403(self): if hasattr(os, 'getuid'): if os.getuid() == 0: raise SkipTest("Can't test permissions as root") if sys.platform.startswith('win'): raise SkipTest("Can't test permissions on Windows") - + with TemporaryDirectory() as td: cm = FileContentsManager(root_dir=td) model = cm.new_untitled(type='file') os_path = cm._get_os_path(model['path']) - + os.chmod(os_path, 0o400) try: with cm.open(os_path, 'w') as f: @@ -234,10 +234,10 @@ class TestContentsManager(TestCase): def tearDown(self): self._temp_dir.cleanup() - + def make_dir(self, api_path): """make a subdirectory at api_path - + override in subclasses if contents are not on the filesystem. """ _make_dir(self.contents_manager, api_path) @@ -283,7 +283,7 @@ class TestContentsManager(TestCase): self.assertEqual(model['name'], 'Untitled Folder') self.assertEqual(model['path'], 'Untitled Folder') sub_dir = model['path'] - + model = cm.new_untitled(path=sub_dir) assert isinstance(model, dict) self.assertIn('name', model) @@ -613,3 +613,17 @@ class TestContentsManager(TestCase): cm.mark_trusted_cells(nb, path) cm.check_and_sign(nb, path) assert cm.notary.check_signature(nb) + + +class TestContentsManagerNoAtomic(TestContentsManager): + """ + Make same test in no atomic case than in atomic case, using inheritance + """ + + def setUp(self): + self._temp_dir = TemporaryDirectory() + self.td = self._temp_dir.name + self.contents_manager = FileContentsManager( + root_dir = self.td, + ) + self.contents_manager.use_atomic_writing = False