From 459fd24c03fe45862f4dcdcc7f93f0261ca6a188 Mon Sep 17 00:00:00 2001 From: Pierre Gerold Date: Mon, 23 Nov 2015 16:06:05 +0100 Subject: [PATCH 1/7] Save without atomic writing and configuration (fix #739) --- notebook/services/contents/fileio.py | 73 +++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 8 deletions(-) diff --git a/notebook/services/contents/fileio.py b/notebook/services/contents/fileio.py index 2fcda7d4a..b86a6c6a8 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,55 @@ 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', **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 + + # Flush to disk + fileobj.flush() + os.fsync(fileobj.fileno()) + fileobj.close() + + + + +class FileManagerMixin(Configurable): """ Mixin for ContentsAPI classes that interact with the filesystem. @@ -119,6 +169,9 @@ class FileManagerMixin(object): log : logging.Logger """ + use_atomic_writing = Bool(True, config=True, help="""flag to deactivate + atomic writing on slow (like networked) file system.""") + @contextmanager def open(self, os_path, *args, **kwargs): """wrapper around io.open that turns permission errors into 403""" @@ -130,8 +183,12 @@ class FileManagerMixin(object): def atomic_writing(self, os_path, *args, **kwargs): """wrapper around atomic_writing that turns permission errors to 403""" 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, **kwargs) as f: + yield f @contextmanager def perm_to_403(self, os_path=''): From f849e310c8cd8e5efe6397b5f21fa9bda618bbb9 Mon Sep 17 00:00:00 2001 From: Pierre Gerold Date: Mon, 23 Nov 2015 16:06:05 +0100 Subject: [PATCH 2/7] Save without atomic writing and configuration (fix #739) --- notebook/services/contents/fileio.py | 73 +++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 8 deletions(-) diff --git a/notebook/services/contents/fileio.py b/notebook/services/contents/fileio.py index 2fcda7d4a..be38fab09 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,55 @@ 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', **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 + + # Flush to disk + fileobj.flush() + os.fsync(fileobj.fileno()) + fileobj.close() + + + + +class FileManagerMixin(Configurable): """ Mixin for ContentsAPI classes that interact with the filesystem. @@ -119,6 +169,9 @@ class FileManagerMixin(object): log : logging.Logger """ + use_atomic_writing = Bool(True, config=True, help="""flag to deactivate + atomic writing on slow (like networked) file system.""") + @contextmanager def open(self, os_path, *args, **kwargs): """wrapper around io.open that turns permission errors into 403""" @@ -130,8 +183,12 @@ class FileManagerMixin(object): def atomic_writing(self, os_path, *args, **kwargs): """wrapper around atomic_writing that turns permission errors to 403""" 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=''): From 48dcf2d3198181e52d1df2d7ffa90d56f0f94272 Mon Sep 17 00:00:00 2001 From: Pierre Gerold Date: Fri, 27 Nov 2015 15:17:57 +0100 Subject: [PATCH 3/7] One carac error + debug logging --- notebook/services/contents/fileio.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/notebook/services/contents/fileio.py b/notebook/services/contents/fileio.py index be38fab09..b9d38726a 100644 --- a/notebook/services/contents/fileio.py +++ b/notebook/services/contents/fileio.py @@ -104,7 +104,7 @@ def atomic_writing(path, text=True, encoding='utf-8', log=None, **kwargs): @contextmanager -def _simple_writing(path, text=True, encoding='utf-8', **kwargs): +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). @@ -144,6 +144,8 @@ def _simple_writing(path, text=True, encoding='utf-8', **kwargs): # Flush to disk fileobj.flush() + if log: + log.debug( path + " saved using simple fs writing") os.fsync(fileobj.fileno()) fileobj.close() @@ -187,7 +189,7 @@ class FileManagerMixin(Configurable): 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: + with _simple_writing(os_path, *args, log=self.log, **kwargs) as f: yield f @contextmanager From 7cfd9b407aa8d8fc9ab4c158ce98c019cf27cc84 Mon Sep 17 00:00:00 2001 From: Pierre Gerold Date: Fri, 27 Nov 2015 18:19:01 +0100 Subject: [PATCH 4/7] Add test for non atomic writing --- .../services/contents/tests/test_manager.py | 51 +++++++++++++++---- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/notebook/services/contents/tests/test_manager.py b/notebook/services/contents/tests/test_manager.py index 3ff2c805f..7b224e4f5 100644 --- a/notebook/services/contents/tests/test_manager.py +++ b/notebook/services/contents/tests/test_manager.py @@ -46,7 +46,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) @@ -106,7 +106,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: @@ -120,7 +120,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: @@ -140,19 +140,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: @@ -188,7 +188,7 @@ class TestFileContentsManager(TestCase): class TestContentsManager(TestCase): - + def setUp(self): self._temp_dir = TemporaryDirectory() self.td = self._temp_dir.name @@ -198,10 +198,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) @@ -247,7 +247,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) @@ -528,3 +528,34 @@ class TestContentsManager(TestCase): cm.mark_trusted_cells(nb, path) cm.check_and_sign(nb, path) assert cm.notary.check_signature(nb) + + +class TestContentsManagerNoAtomic(TestCase): + + 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 + + def tearDown(self): + self._temp_dir.cleanup() + + def new_notebook_no_atomic(self): + """ + + """ + cm = self.contents_manager + model = cm.new_untitled(type="notebook") + name = model['name'] + path = model['path'] + + full_model = cm.get(path) + nb = full_model['content'] + nb['metadata']['counter'] = int(1e6 * time.time()) + self.add_code_cell(nb) + + cm.save(full_model, path) + return nb, name, path From 28d6fbde8481ecf61e2dc6d4ad77f82e08aee44f Mon Sep 17 00:00:00 2001 From: Pierre Gerold Date: Mon, 7 Dec 2015 10:57:51 +0100 Subject: [PATCH 5/7] Modify test + correct docstrings --- notebook/services/contents/fileio.py | 10 +++++--- .../services/contents/tests/test_manager.py | 25 +++---------------- 2 files changed, 11 insertions(+), 24 deletions(-) diff --git a/notebook/services/contents/fileio.py b/notebook/services/contents/fileio.py index b9d38726a..003491194 100644 --- a/notebook/services/contents/fileio.py +++ b/notebook/services/contents/fileio.py @@ -171,8 +171,10 @@ class FileManagerMixin(Configurable): log : logging.Logger """ - use_atomic_writing = Bool(True, config=True, help="""flag to deactivate - atomic writing on slow (like networked) file system.""") + 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): @@ -183,7 +185,9 @@ class FileManagerMixin(Configurable): @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): if self.use_atomic_writing: with atomic_writing(os_path, *args, log=self.log, **kwargs) as f: diff --git a/notebook/services/contents/tests/test_manager.py b/notebook/services/contents/tests/test_manager.py index 7b224e4f5..7954f6726 100644 --- a/notebook/services/contents/tests/test_manager.py +++ b/notebook/services/contents/tests/test_manager.py @@ -530,7 +530,10 @@ class TestContentsManager(TestCase): assert cm.notary.check_signature(nb) -class TestContentsManagerNoAtomic(TestCase): +class TestContentsManagerNoAtomic(TestContentsManager): + """ + Make same test in no atomic case than in atomic case, using inheritance + """ def setUp(self): self._temp_dir = TemporaryDirectory() @@ -539,23 +542,3 @@ class TestContentsManagerNoAtomic(TestCase): root_dir = self.td, ) self.contents_manager.use_atomic_writing = False - - def tearDown(self): - self._temp_dir.cleanup() - - def new_notebook_no_atomic(self): - """ - - """ - cm = self.contents_manager - model = cm.new_untitled(type="notebook") - name = model['name'] - path = model['path'] - - full_model = cm.get(path) - nb = full_model['content'] - nb['metadata']['counter'] = int(1e6 * time.time()) - self.add_code_cell(nb) - - cm.save(full_model, path) - return nb, name, path From fbcf2c403e6df973aba45210920bd5ae19110693 Mon Sep 17 00:00:00 2001 From: Pierre Gerold Date: Mon, 7 Dec 2015 11:52:52 +0100 Subject: [PATCH 6/7] remove flush after save (no atomic writing) --- notebook/services/contents/fileio.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/notebook/services/contents/fileio.py b/notebook/services/contents/fileio.py index 003491194..9dd283572 100644 --- a/notebook/services/contents/fileio.py +++ b/notebook/services/contents/fileio.py @@ -142,11 +142,6 @@ def _simple_writing(path, text=True, encoding='utf-8', log=None, **kwargs): fileobj.close() raise - # Flush to disk - fileobj.flush() - if log: - log.debug( path + " saved using simple fs writing") - os.fsync(fileobj.fileno()) fileobj.close() From 6ba2104c4b4f8b8d6a8af221b93b36f5f8bbe6d9 Mon Sep 17 00:00:00 2001 From: Pierre Gerold Date: Mon, 7 Dec 2015 12:10:18 +0100 Subject: [PATCH 7/7] finish the merge ... yeh I missed a <<<<<<>>>>>> master """Context manager to write file without doing atomic writing ( for weird filesystem eg: nfs).