Merge pull request #747 from parleur/toto

Allow disabling atomic writing

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=''):

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