Rev 5362: More doc and ensure that the config is locked when _write_config_file is called. 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: 5362
revision-id: v.ladeuil+lp at free.fr-20100823140433-03cyut95alojc8be
parent: v.ladeuil+lp at free.fr-20100823124549-uoszb3y8ih24kncm
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: lockable-config-files
timestamp: Mon 2010-08-23 16:04:33 +0200
message:
  More doc and ensure that the config is locked when _write_config_file is called.
-------------- next part --------------
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py	2010-08-23 12:45:49 +0000
+++ b/bzrlib/config.py	2010-08-23 14:04:33 +0000
@@ -535,6 +535,24 @@
 
     If several processes try to write the config file, the accesses need to be
     serialized.
+
+    Daughter classes should decorate all methods that update a config with the
+    ``@needs_write_lock`` decorator (they call, directly or indirectly, the
+    ``_write_config_file()`` method. These methods (typically ``set_option()``
+    and variants must reload the config file from disk before calling
+    ``_write_config_file()``), this can be achieved by calling the
+    ``self.reload()`` method. Note that the lock scope should cover both the
+    reading and the writing of the config file which is why the decorator can't
+    be applied to ``_write_config_file()`` only.
+
+    This should be enough to implement the following logic:
+    - lock for exclusive write access,
+    - reload the config file from disk,
+    - set the new value
+    - unlock
+
+    This logic guarantees that a writer can update a value without erasing an
+    update made by another writer.
     """
 
     lock_name = 'lock'
@@ -547,12 +565,23 @@
         self._lock = lockdir.LockDir(self.transport, 'lock')
 
     def lock_write(self, token=None):
+        """Takes a write lock in the directory containing the config file.
+
+        If the directory doesn't exist it is created.
+        """
         ensure_config_dir_exists(self.dir)
         return self._lock.lock_write(token)
 
     def unlock(self):
         self._lock.unlock()
 
+    def _write_config_file(self):
+        if self._lock is None or not self._lock.is_held:
+            # NB: if the following exception is raised it probably means a
+            # missing @needs_write_lock decorator on one of the callers.
+            raise errors.ObjectNotLocked(self)
+        super(LockableConfig, self)._write_config_file()
+
 
 class GlobalConfig(LockableConfig):
     """The configuration that should be used for a specific location."""
@@ -576,10 +605,12 @@
         else:
             return {}
 
+    @needs_write_lock
     def set_alias(self, alias_name, alias_command):
         """Save the alias in the configuration."""
         self._set_option(alias_name, alias_command, 'ALIASES')
 
+    @needs_write_lock
     def unset_alias(self, alias_name):
         """Unset an existing alias."""
         aliases = self._get_parser().get('ALIASES')

=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py	2010-08-23 12:45:49 +0000
+++ b/bzrlib/tests/test_config.py	2010-08-23 14:04:33 +0000
@@ -417,8 +417,7 @@
         self.path, self.uid, self.gid = path, uid, gid
 
     def test_ini_config_ownership(self):
-        """Ensure that chown is happening during _write_config_file.
-        """
+        """Ensure that chown is happening during _write_config_file"""
         self.requireFeature(features.chown_feature)
         self.overrideAttr(os, 'chown', self._dummy_chown)
         self.path = self.uid = self.gid = None
@@ -485,7 +484,9 @@
 
     def create_config(self, content):
         c = self.config_class(*self.config_args, _content=content)
+        c.lock_write()
         c._write_config_file()
+        c.unlock()
         return c
 
     def test_simple_read_access(self):
@@ -1166,10 +1167,14 @@
             global_config = sample_config_text
 
         my_global_config = config.GlobalConfig(_content=global_config)
+        my_global_config.lock_write()
         my_global_config._write_config_file()
+        my_global_config.unlock()
         my_location_config = config.LocationConfig(
             my_branch.base, _content=sample_branches_text)
+        my_location_config.lock_write()
         my_location_config._write_config_file()
+        my_location_config.unlock()
 
         my_config = config.BranchConfig(my_branch)
         self.my_config = my_config
@@ -1242,11 +1247,15 @@
         my_branch = FakeBranch(location)
         if global_config is not None:
             my_global_config = config.GlobalConfig(_content=global_config)
+            my_global_config.lock_write()
             my_global_config._write_config_file()
+            my_global_config.unlock()
         if location_config is not None:
             my_location_config = config.LocationConfig(my_branch.base,
                                                        _content=location_config)
+            my_location_config.lock_write()
             my_location_config._write_config_file()
+            my_location_config.unlock()
         my_config = config.BranchConfig(my_branch)
         if branch_data_config is not None:
             my_config.branch.control_files.files['branch.conf'] = \