Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

conf: don't overwrite a configuration file on serialization error #22

Merged
merged 2 commits into from

2 participants

@vincentbernat

When a serilization error occurs, the resulting file is empty. We wrap
the file object to write to a temporary file and overwrite the file
only when the whole process has succeeded.

This only works for file-based backend (not directories). YAML is not tested because I didn't find out how to have a serialization error in YAML

@vincentbernat vincentbernat conf: don't overwrite a configuration file on serialization error
When a serilization error occurs, the resulting file is empty. We wrap
the file object to write to a temporary file and overwrite the file
only when the whole process has succeeded.
5f18526
@rs
Owner
rs commented

Are your sure wiping the file isn't the best approach?

@vincentbernat

Let's talk about this tomorrow if you want. Not sure to understand what you mean by wipe.

@vincentbernat

I think this "fix" hides another problem. Hold it for now.

@vincentbernat

The following race condition may be raised: target file is opened for writing, it is truncated and it is for a few moments empty. It is easy for us to observe this problem. It is odd that it is happening so often because such a window should be very short. Maybe Python is not really closing the file at the end of the with statement but I checked in the source code and f.close() is called on __exit__. So dunno why, but this happens and this is also fixed by this patch.

@rs rs merged commit 987d2d3 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 22, 2013
  1. @vincentbernat

    conf: don't overwrite a configuration file on serialization error

    vincentbernat authored
    When a serilization error occurs, the resulting file is empty. We wrap
    the file object to write to a temporary file and overwrite the file
    only when the whole process has succeeded.
Commits on Feb 23, 2013
  1. @vincentbernat
This page is out of date. Refresh to see the latest.
Showing with 62 additions and 4 deletions.
  1. +39 −0 tests/test_conf.py
  2. +23 −4 zkfarmer/conf.py
View
39 tests/test_conf.py
@@ -5,6 +5,7 @@
import yaml
import os
import sys
+import stat
from mock import patch
from zkfarmer import conf
@@ -89,6 +90,35 @@ def test_json_write_to_stdout(self):
self.assertEqual(json.load(f),
{"1": "2"})
+ def test_json_no_overwrite_on_failure(self):
+ """Check we don't overwrite an existing file in case of failure."""
+ name = "%s/test.json" % self.tmpdir
+ a = conf.Conf(name)
+ a.write({"1": "2"})
+ # Write something invalid
+ self.assertRaises(TypeError, a.write, json)
+ self.assertEqual(a.read(), {"1": "2"})
+
+ def test_json_appropriate_rights(self):
+ """Check if a file is created with the appropriate rights"""
+ os.umask(022)
+ name = "%s/test.json" % self.tmpdir
+ a = conf.Conf(name)
+ a.write({"1": "2"})
+ del a
+ a = os.stat(name)
+ self.assertEqual(a.st_mode & 0777, 0644)
+
+ def test_json_appropriate_rights_umask(self):
+ """Check if a file is created with appropriate rights using non standard umask."""
+ os.umask(027)
+ name = "%s/test.json" % self.tmpdir
+ a = conf.Conf(name)
+ a.write({"1": "2"})
+ del a
+ a = os.stat(name)
+ self.assertEqual(a.st_mode & 0777, 0640)
+
class TestConfYAML(TempDirectoryTestCase):
def test_yaml_write_from_extension(self):
@@ -176,6 +206,15 @@ def test_php_read(self):
with self.assertRaises(NotImplementedError):
conf.Conf(name).read()
+ def test_php_no_overwrite_on_failure(self):
+ """Check we don't overwrite an existing file in case of failure."""
+ name = "%s/test.json" % self.tmpdir
+ a = conf.Conf(name)
+ a.write({"1": "2"})
+ # Write something invalid
+ self.assertRaises(TypeError, a.write, json)
+ self.assertEqual(a.read(), {"1": "2"})
+
class TestConfDir(TempDirectoryTestCase):
def test_dir_from_existence(self):
View
27 zkfarmer/conf.py
@@ -11,6 +11,8 @@
import shutil
import json
import yaml
+import contextlib
+import tempfile
# Prevent unstandard !!python/unicode prefixes
yaml.add_representer(unicode, lambda dumper, value: dumper.represent_scalar(u'tag:yaml.org,2002:str', value))
@@ -55,15 +57,32 @@ class ConfFile(ConfBase):
def __init__(self, file_path):
self.file_path = file_path
+ @contextlib.contextmanager
def open(self, write=False):
if self.file_path == '-':
if write:
- return sys.stdout
+ try:
+ yield sys.stdout
+ finally:
+ sys.stdout.flush()
+ return
else:
raise NotImplementedError('Cannot read configuration from stdin')
- mode = 'w' if write else 'r'
- return open(self.file_path, mode)
-
+ if not write:
+ yield open(self.file_path, 'r')
+ return
+ tmp, tmpname = tempfile.mkstemp(dir=os.path.dirname(os.path.abspath(self.file_path)))
+ try:
+ current_umask = os.umask(0)
+ os.umask(current_umask)
+ os.chmod(tmpname, 0666 & ~current_umask)
+ f = os.fdopen(tmp, "w")
+ yield f
+ f.close()
+ os.rename(tmpname, self.file_path)
+ except:
+ os.unlink(tmpname)
+ raise
class ConfJSON(ConfFile):
def read(self):
Something went wrong with that request. Please try again.