Rev 5358: Start implementing config files locking. 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: 5358
revision-id: v.ladeuil+lp at free.fr-20100722083545-1qvav61t0d4rrojc
parent: v.ladeuil+lp at free.fr-20100722081215-qvjirmudwj527spb
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: lockable-config-files
timestamp: Thu 2010-07-22 10:35:45 +0200
message:
  Start implementing config files locking.
  
  * bzrlib/tests/test_config.py:
  (TestLockableConfig): Add a test for concurrent writers.
  
  * bzrlib/config.py:
  (LockableConfig): Some configs need to be locked (GlobalConfig and
  LocationConfig at least).
  (GlobalConfig.set_user_option): Needs a write lock to protect
  against concurrent writes.
-------------- next part --------------
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py	2010-07-22 07:56:13 +0000
+++ b/bzrlib/config.py	2010-07-22 08:35:45 +0000
@@ -65,6 +65,7 @@
 import os
 import sys
 
+from bzrlib.decorators import needs_write_lock
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
 import errno
@@ -77,11 +78,13 @@
     atomicfile,
     debug,
     errors,
+    lockdir,
     mail_client,
     osutils,
     registry,
     symbol_versioning,
     trace,
+    transport,
     ui,
     urlutils,
     win32utils,
@@ -526,7 +529,28 @@
         atomic_file.close()
 
 
-class GlobalConfig(IniBasedConfig):
+class LockableConfig(IniBasedConfig):
+    """A configuration needing explicit locking for access.
+
+    If several processes try to write the config file, the accesses need to be
+    serialized.
+    """
+
+    def __init__(self, file_name, _content=None):
+        super(LockableConfig, self).__init__(file_name=file_name,
+                                             _content=_content)
+        t = transport.get_transport(config_dir())
+        self._lock = lockdir.LockDir(t, 'lock')
+
+    def lock_write(self, token=None):
+        ensure_config_dir_exists(config_dir())
+        return self._lock.lock_write(token)
+
+    def unlock(self):
+        self._lock.unlock()
+
+
+class GlobalConfig(LockableConfig):
     """The configuration that should be used for a specific location."""
 
     def __init__(self, _content=None):
@@ -536,6 +560,7 @@
     def get_editor(self):
         return self._get_user_option('editor')
 
+    @needs_write_lock
     def set_user_option(self, option, value):
         """Save option and its value in the configuration."""
         self._set_option(option, value, 'DEFAULT')
@@ -560,7 +585,6 @@
         self._write_config_file()
 
     def _set_option(self, option, value, section):
-        # FIXME: RBC 20051029 This should take a file lock on bazaar.conf.
         self.reload()
         self._get_parser().setdefault(section, {})[option] = value
         self._write_config_file()

=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py	2010-07-22 08:12:15 +0000
+++ b/bzrlib/tests/test_config.py	2010-07-22 08:35:45 +0000
@@ -19,6 +19,7 @@
 from cStringIO import StringIO
 import os
 import sys
+import threading
 
 #import bzrlib specific imports here
 from bzrlib import (
@@ -472,6 +473,42 @@
         c1.set_user_option('two', 'done')
         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)
+
+        # We spawn a thread that will pause *during* the write
+        before_writing = threading.Event()
+        after_writing = threading.Event()
+        writing_done = threading.Event()
+        c1_orig = c1._write_config_file
+        def c1_write_config_file():
+            before_writing.set()
+            c1_orig()
+            # The lock is held we wait for the main thread to decide when to
+            # continue
+            after_writing.wait()
+        c1._write_config_file = c1_write_config_file
+        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(after_writing.set)
+        t1.start()
+        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
+        after_writing.set()
+        writing_done.wait()
+        c2.set_user_option('one', 'c2')
+        self.assertEquals('c2', c2.get_user_option('one'))
+
 
 class TestGetUserOptionAs(TestIniConfig):