Rev 5324: Fix fallouts. in file:///home/vila/src/bzr/bugs/525571-lock-bazaar-conf-files/

Vincent Ladeuil v.ladeuil+lp at free.fr
Tue Jun 29 10:19:11 BST 2010


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

------------------------------------------------------------
revno: 5324
revision-id: v.ladeuil+lp at free.fr-20100629091911-uuzfksmfm0z9oxtq
parent: v.ladeuil+lp at free.fr-20100629073659-z7cmys2po4ztabr3
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: 525571-lock-bazaar-conf-files
timestamp: Tue 2010-06-29 11:19:11 +0200
message:
  Fix fallouts.
  
  * bzrlib/tests/test_config.py:
  (TestLockableConfig.test_writes_are_serialized): Fix a race
  condition introduced by the refactoring and override the _atomic
  method.
  
  * bzrlib/config.py:
  (IniBasedConfig._get_parser): Once the config file has been
  parsed, the filename (if available) should be set so that reload()
  can be used.
  (LockableConfig): Simplify the implementation.
  (LockableConfig._write_config_file_atomic): For test purposes, we
  need to separate the method receiving the needs_write_lock
  decorator from the method that actually implements the atomic
  write.
-------------- next part --------------
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py	2010-06-29 07:36:59 +0000
+++ b/bzrlib/config.py	2010-06-29 09:19:11 +0000
@@ -78,6 +78,7 @@
 from bzrlib import (
     debug,
     errors,
+    lock,
     lockdir,
     mail_client,
     osutils,
@@ -363,15 +364,24 @@
     def _get_parser(self, infile=None):
         if self._parser is not None:
             return self._parser
+        if self._get_filename is None:
+            fname = None
+        else:
+            fname = self._get_filename()
         if infile is None:
-            fname_or_content = self._get_filename()
+            fname_or_content = fname
         else:
             fname_or_content = infile
+        p = None
         try:
-            self._parser = ConfigObj(fname_or_content, encoding='utf-8')
+            p = ConfigObj(fname_or_content, encoding='utf-8')
         except configobj.ConfigObjError, e:
             raise errors.ParseConfigError(e.errors, e.config.filename)
-        return self._parser
+        if p is not None and fname is not None:
+            # Make sure p.reload() will use the right file name
+            p.filename = fname
+        self._parser = p
+        return p
 
     def _get_matching_sections(self):
         """Return an ordered list of (section_name, extra_path) pairs.
@@ -500,27 +510,29 @@
     serialized.
     """
 
-    def __init__(self, get_filename):
-        super(LockableConfig, self).__init__(get_filename)
-        self.transport = transport.get_transport(config_dir())
+    def lock_write(self, token=None):
+        conf_dir = os.path.dirname(self._get_filename())
+        ensure_config_dir_exists(conf_dir)
+        self.transport = transport.get_transport(conf_dir)
         self._lock = lockdir.LockDir(self.transport, 'lock')
-
-    def lock_write(self, token=None):
-        ensure_config_dir_exists(config_dir())
-        return self._lock.lock_write(token)
+        self._lock.lock_write(token)
+        return lock.LogicalLockResult(self.unlock)
 
     def unlock(self):
         self._lock.unlock()
 
+    @needs_write_lock
     def _write_config_file(self):
+        self._write_config_file_atomic()
+
+    def _write_config_file_atomic(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)
+        p = self._get_parser()
+        p.write(f)
         self.transport.put_bytes(os.path.basename(fname), f.getvalue())
+        # Make sure p.reload() will use the right file name
+        p.filename = fname
         osutils.copy_ownership_from_path(fname)
 
 
@@ -530,7 +542,6 @@
     def __init__(self):
         super(GlobalConfig, self).__init__(config_filename)
 
-    @needs_write_lock
     def set_user_option(self, option, value):
         """Save option and its value in the configuration."""
         self._set_option(option, value, 'DEFAULT')
@@ -680,7 +691,6 @@
             if policy_key in self._get_parser()[section]:
                 del self._get_parser()[section][policy_key]
 
-    @needs_write_lock
     def set_user_option(self, option, value, store=STORE_LOCATION):
         """Save option and its value in the configuration."""
         if store not in [STORE_LOCATION,

=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py	2010-06-29 07:36:59 +0000
+++ b/bzrlib/tests/test_config.py	2010-06-29 09:19:11 +0000
@@ -479,28 +479,34 @@
         c2 = self.create_config()
 
         # We spawn a thread that will pause *during* the write
-        c1_before_writing = threading.Event()
-        c1_after_writing = threading.Event()
-        c1_orig = c1._write_config_file
-        def c1_write_config_file():
-            c1_before_writing.set()
+        before_writing = threading.Event()
+        after_writing = threading.Event()
+        writing_done = threading.Event()
+        c1_orig = c1._write_config_file_atomic
+        def c1_write_config_file_atomic():
+            before_writing.set()
             c1_orig()
             # The lock is held we wait for the main thread to decide when to
             # continue
-            c1_after_writing.wait()
-        c1._write_config_file = c1_write_config_file
+            after_writing.wait()
+        c1._write_config_file_atomic = c1_write_config_file_atomic
         def c1_set_option():
             c1.set_user_option('one', 'c1')
+            writing_done.set()
         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(writing_done.set)
         t1.start()
-        c1_before_writing.wait()
+        before_writing.wait()
         self.assertTrue(c1._lock.is_held)
         self.assertRaises(errors.LockContention,
                           c2.set_user_option, 'one', 'c2')
         self.assertEquals('c1', c1.get_user_option('one'))
         # Let the lock be released
-        c1_after_writing.set()
+        after_writing.set()
+        writing_done.wait()
         c2.set_user_option('one', 'c2')
         self.assertEquals('c2', c2.get_user_option('one'))
 
@@ -510,15 +516,15 @@
         ready_to_write = threading.Event()
         do_writing = threading.Event()
         writing_done = threading.Event()
-        c1_orig = c1._write_config_file
-        def c1_write_config_file():
+        c1_orig = c1._write_config_file_atomic
+        def c1_write_config_file_atomic():
             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
+        c1._write_config_file_atomic = c1_write_config_file_atomic
         def c1_set_option():
             c1.set_user_option('one', 'c1')
         t1 = threading.Thread(target=c1_set_option)



More information about the bazaar-commits mailing list