Rev 5323: Ensure lockable config files are atomically written. in file:///home/vila/src/bzr/bugs/525571-lock-bazaar-conf-files/

[Vincent Ladeuil]

At file:///home/vila/src/bzr/bugs/525571-lock-bazaar-conf-files/

------------------------------------------------------------
revno: 5323
revision-id: v.ladeuil+lp at free.fr-20100629073659-z7cmys2po4ztabr3
parent: v.ladeuil+lp at free.fr-20100628155749-mtntt9jcdvtag69v
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: 525571-lock-bazaar-conf-files
timestamp: Tue 2010-06-29 09:36:59 +0200
message:
  Ensure lockable config files are atomically written.
  
  * bzrlib/tests/test_config.py:
  (TestLockableConfig.test_read_while_writing): Ensure the readers
  gets the most recent version and that's its valid.
  
  * bzrlib/config.py:
  (LockableConfig._write_config_file): An atomic version to ensure
  readers don't get partial files.
-------------- next part --------------
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py	2010-06-28 15:57:49 +0000
+++ b/bzrlib/config.py	2010-06-29 07:36:59 +0000
@@ -62,6 +62,7 @@
 up=pull
 """
 
+from cStringIO import StringIO
 import os
 import sys
 
@@ -501,8 +502,8 @@
 
     def __init__(self, get_filename):
         super(LockableConfig, self).__init__(get_filename)
-        t = transport.get_transport(config_dir())
-        self._lock = lockdir.LockDir(t, 'lock')
+        self.transport = transport.get_transport(config_dir())
+        self._lock = lockdir.LockDir(self.transport, 'lock')
 
     def lock_write(self, token=None):
         ensure_config_dir_exists(config_dir())
@@ -511,6 +512,17 @@
     def unlock(self):
         self._lock.unlock()
 
+    def _write_config_file(self):
+        fname = self._get_filename()
+        conf_dir = os.path.dirname(fname)
+        # the transport API won't take the windows special case into account
+        # here (see ensure_config_dir_exists).
+        ensure_config_dir_exists(conf_dir)
+        f = StringIO()
+        self._get_parser().write(f)
+        self.transport.put_bytes(os.path.basename(fname), f.getvalue())
+        osutils.copy_ownership_from_path(fname)
+
 
 class GlobalConfig(LockableConfig):
     """The configuration that should be used for a specific location."""
@@ -546,8 +558,6 @@
         self._write_config_file()
 
     def _set_option(self, option, value, section):
-        conf_dir = os.path.dirname(self._get_filename())
-        ensure_config_dir_exists(conf_dir)
         if self._parser is not None:
             self._parser.reload()
         self._get_parser().setdefault(section, {})[option] = value

=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py	2010-06-28 15:57:49 +0000
+++ b/bzrlib/tests/test_config.py	2010-06-29 07:36:59 +0000
@@ -504,6 +504,43 @@
         c2.set_user_option('one', 'c2')
         self.assertEquals('c2', c2.get_user_option('one'))
 
+    def test_read_while_writing(self):
+        c1 = self.create_config()
+        # We spawn a thread that will pause *during* the write
+        ready_to_write = threading.Event()
+        do_writing = threading.Event()
+        writing_done = threading.Event()
+        c1_orig = c1._write_config_file
+        def c1_write_config_file():
+            ready_to_write.set()
+            # The lock is held we wait for the main thread to decide when to
+            # continue
+            do_writing.wait()
+            c1_orig()
+            writing_done.set()
+        c1._write_config_file = c1_write_config_file
+        def c1_set_option():
+            c1.set_user_option('one', 'c1')
+        t1 = threading.Thread(target=c1_set_option)
+        # Collect the thread after the test
+        self.addCleanup(t1.join)
+        # Be ready to unblock the thread if the test goes wrong
+        self.addCleanup(do_writing.set)
+        t1.start()
+        # Ensure the thread is ready to write
+        ready_to_write.wait()
+        self.assertTrue(c1._lock.is_held)
+        self.assertEquals('c1', c1.get_user_option('one'))
+        # If we read during the write, we get the old value
+        c2 = self.create_config()
+        self.assertEquals('1', c2.get_user_option('one'))
+        # Let the writing occur and ensure it occurred
+        do_writing.set()
+        writing_done.wait()
+        # Now we get the updated value
+        c3 = self.create_config()
+        self.assertEquals('c1', c3.get_user_option('one'))
+
 
 class TestGetUserOptionAs(TestIniConfig):