Rev 5361: Make LocationConfig use a lock too. in file:///home/vila/src/bzr/bugs/525571-lock-bazaar-conf-files/

Vincent Ladeuil v.ladeuil+lp at free.fr
Mon Aug 23 13:45:50 BST 2010


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

------------------------------------------------------------
revno: 5361
revision-id: v.ladeuil+lp at free.fr-20100823124549-uoszb3y8ih24kncm
parent: v.ladeuil+lp at free.fr-20100823093203-gndeu5r71wft0pl6
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: lockable-config-files
timestamp: Mon 2010-08-23 14:45:49 +0200
message:
  Make LocationConfig use a lock too.
  
  * bzrlib/tests/test_config.py:
  (lockable_config_scenarios, load_tests): What configs should be
  tested as lockable ones.
  (TestIniConfigBuilding.test_ini_config_ownership): Fixed.
  (TestLockableConfig.get_existing_config): Some tests need to load
  an existing config file rather than creating it.
  (TestLockableConfig.test_read_while_writing): Ensure we get the
  old config until the lock is released.
-------------- next part --------------
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py	2010-08-23 09:32:03 +0000
+++ b/bzrlib/config.py	2010-08-23 12:45:49 +0000
@@ -594,7 +594,7 @@
         self._write_config_file()
 
 
-class LocationConfig(IniBasedConfig):
+class LocationConfig(LockableConfig):
     """A configuration object that gives the policy for a location."""
 
     def __init__(self, location, _content=None):
@@ -701,6 +701,7 @@
             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,
@@ -709,7 +710,6 @@
             raise ValueError('bad storage policy %r for %r' %
                 (store, option))
         self.reload()
-        # FIXME: RBC 20051029 This should take a file lock on locations.conf.
         location = self.location
         if location.endswith('/'):
             location = location[:-1]

=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py	2010-08-23 09:32:03 +0000
+++ b/bzrlib/tests/test_config.py	2010-08-23 12:45:49 +0000
@@ -40,6 +40,30 @@
 from bzrlib.util.configobj import configobj
 
 
+def lockable_config_scenarios():
+    return [
+        ('global',
+         {'config_class': config.GlobalConfig,
+          'config_args': [],
+          'config_section': 'DEFAULT'}),
+        ('locations',
+         {'config_class': config.LocationConfig,
+          'config_args': ['.'],
+          'config_section': '.'}),]
+
+
+def load_tests(standard_tests, module, loader):
+    suite = loader.suiteClass()
+
+    lc_tests, remaining_tests = tests.split_suite_by_condition(
+        standard_tests, tests.condition_isinstance((
+                TestLockableConfig,
+                )))
+    tests.multiply_tests(lc_tests, lockable_config_scenarios(), suite)
+    suite.addTest(remaining_tests)
+    return suite
+
+
 sample_long_alias="log -r-15..-1 --line"
 sample_config_text = u"""
 [DEFAULT]
@@ -398,9 +422,9 @@
         self.requireFeature(features.chown_feature)
         self.overrideAttr(os, 'chown', self._dummy_chown)
         self.path = self.uid = self.gid = None
-        conf = config.IniBasedConfig(file_name='foo.conf')
+        conf = config.IniBasedConfig(file_name='./foo.conf')
         conf._write_config_file()
-        self.assertEquals(self.path, 'foo.conf')
+        self.assertEquals(self.path, './foo.conf')
         self.assertTrue(isinstance(self.uid, int))
         self.assertTrue(isinstance(self.gid, int))
 
@@ -446,15 +470,21 @@
 
 class TestLockableConfig(tests.TestCaseInTempDir):
 
-    config_class = config.GlobalConfig
+    # Set by load_tests
+    config_class = None
+    config_args = None
+    config_section = None
 
     def setUp(self):
         super(TestLockableConfig, self).setUp()
-        self._content = '[DEFAULT]\none=1\ntwo=2'
+        self._content = '[%s]\none=1\ntwo=2\n' % (self.config_section,)
         self.config = self.create_config(self._content)
 
+    def get_existing_config(self):
+        return self.config_class(*self.config_args)
+
     def create_config(self, content):
-        c = self.config_class(_content=content)
+        c = self.config_class(*self.config_args, _content=content)
         c._write_config_file()
         return c
 
@@ -467,7 +497,7 @@
 
     def test_listen_to_the_last_speaker(self):
         c1 = self.config
-        c2 = self.create_config(self._content)
+        c2 = self.get_existing_config()
         c1.set_user_option('one', 'ONE')
         c2.set_user_option('two', 'TWO')
         self.assertEquals('ONE', c1.get_user_option('one'))
@@ -479,7 +509,7 @@
         # If the same config is not shared, the same variable modified twice
         # can only see a single result.
         c1 = self.config
-        c2 = self.create_config(self._content)
+        c2 = self.get_existing_config()
         c1.set_user_option('one', 'c1')
         c2.set_user_option('one', 'c2')
         self.assertEquals('c2', c2._get_user_option('one'))
@@ -490,8 +520,8 @@
         self.assertEquals('c2', c1._get_user_option('one'))
 
     def test_writes_are_serialized(self):
-        c1 = self.create_config(self._content)
-        c2 = self.create_config(self._content)
+        c1 = self.config
+        c2 = self.get_existing_config()
 
         # We spawn a thread that will pause *during* the write
         before_writing = threading.Event()
@@ -525,6 +555,43 @@
         c2.set_user_option('one', 'c2')
         self.assertEquals('c2', c2.get_user_option('one'))
 
+    def test_read_while_writing(self):
+       c1 = self.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.get_existing_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.get_existing_config()
+       self.assertEquals('c1', c3.get_user_option('one'))
+
 
 class TestGetUserOptionAs(TestIniConfig):
 



More information about the bazaar-commits mailing list