Rev 5322: Protect config files against concurrent writers. in file:///home/vila/src/bzr/bugs/525571-lock-bazaar-conf-files/
Vincent Ladeuil
v.ladeuil+lp at free.fr
Mon Jun 28 16:57:49 BST 2010
At file:///home/vila/src/bzr/bugs/525571-lock-bazaar-conf-files/
------------------------------------------------------------
revno: 5322
revision-id: v.ladeuil+lp at free.fr-20100628155749-mtntt9jcdvtag69v
parent: v.ladeuil+lp at free.fr-20100628085603-15lecj3pc3nduat6
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: 525571-lock-bazaar-conf-files
timestamp: Mon 2010-06-28 17:57:49 +0200
message:
Protect config files against concurrent writers.
* bzrlib/tests/test_config.py:
(TestLockableConfig): Apply the tests to both GlobalConfig and
LocationConfig.
* bzrlib/config.py:
(LockableConfig): Some configs need to be locked (GlobalConfig and
LocationConfig at least).
(GlobalConfig.set_user_option, LocationConfig.set_user_option):
Needs a write lock to protect against concurrent writes.
-------------- next part --------------
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py 2010-06-28 08:51:22 +0000
+++ b/bzrlib/config.py 2010-06-28 15:57:49 +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
@@ -76,11 +77,13 @@
from bzrlib import (
debug,
errors,
+ lockdir,
mail_client,
osutils,
registry,
symbol_versioning,
trace,
+ transport,
ui,
urlutils,
win32utils,
@@ -489,19 +492,40 @@
f.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, get_filename):
+ super(LockableConfig, self).__init__(get_filename)
+ 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 get_editor(self):
- return self._get_user_option('editor')
-
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')
+ def get_editor(self):
+ return self._get_user_option('editor')
+
def get_aliases(self):
"""Return the aliases section."""
if 'ALIASES' in self._get_parser():
@@ -522,8 +546,6 @@
self._write_config_file()
def _set_option(self, option, value, section):
- # FIXME: RBC 20051029 This should refresh the parser and also take a
- # file lock on bazaar.conf.
conf_dir = os.path.dirname(self._get_filename())
ensure_config_dir_exists(conf_dir)
if self._parser is not None:
@@ -532,7 +554,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):
@@ -648,6 +670,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,
@@ -657,8 +680,6 @@
(store, option))
if self._parser is not None:
self._parser.reload()
- # FIXME: RBC 20051029 This should refresh the parser and also 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-06-28 08:56:03 +0000
+++ b/bzrlib/tests/test_config.py 2010-06-28 15:57:49 +0000
@@ -19,6 +19,7 @@
from cStringIO import StringIO
import os
import sys
+import threading
#import bzrlib specific imports here
from bzrlib import (
@@ -38,6 +39,32 @@
from bzrlib.util.configobj import configobj
+def lockable_config_scenarios():
+ return [
+ ('global',
+ {'config_file_name': config.config_filename,
+ 'config_class': config.GlobalConfig,
+ 'config_args': [],
+ 'config_section': 'DEFAULT'}),
+ ('locations',
+ {'config_file_name': config.locations_config_filename,
+ '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]
@@ -399,50 +426,83 @@
class TestLockableConfig(tests.TestCaseInTempDir):
- config_class = config.GlobalConfig
+ # Set by load_tests
+ config_file_name = None
+ config_class = None
+ config_args = None
+ config_section = None
def setUp(self):
super(TestLockableConfig, self).setUp()
config.ensure_config_dir_exists()
- c = self.config_class()
- fname = c._get_filename()
- self.build_tree_contents([(fname, '''[DEFAULT]\none=1\ntwo=2''')])
- self.config = self.create_config()
+ text = '[%s]\none=1\ntwo=2\n' % (self.config_section,)
+ self.build_tree_contents([(self.config_file_name(), text)])
def create_config(self):
- c = self.config_class()
- return c
+ return self.config_class(*self.config_args)
def test_simple_read_access(self):
- self.assertEquals('1', self.config._get_user_option('one'))
+ c = self.create_config()
+ self.assertEquals('1', c.get_user_option('one'))
def test_simple_write_access(self):
- self.config.set_user_option('one', 'one')
- self.assertEquals('one', self.config._get_user_option('one'))
+ c = self.create_config()
+ c.set_user_option('one', 'one')
+ self.assertEquals('one', c.get_user_option('one'))
def test_listen_to_the_last_speaker(self):
- c1 = self.config
+ c1 = self.create_config()
c2 = self.create_config()
c1.set_user_option('one', 'ONE')
c2.set_user_option('two', 'TWO')
- self.assertEquals('ONE', c1._get_user_option('one'))
- self.assertEquals('TWO', c2._get_user_option('two'))
+ self.assertEquals('ONE', c1.get_user_option('one'))
+ self.assertEquals('TWO', c2.get_user_option('two'))
# The second update respect the first one
- self.assertEquals('ONE', c2._get_user_option('one'))
+ self.assertEquals('ONE', c2.get_user_option('one'))
def test_last_speaker_wins(self):
# If the same config is not shared, the same variable modified twice
# can only see a single result.
- c1 = self.config
+ c1 = self.create_config()
c2 = self.create_config()
c1.set_user_option('one', 'c1')
c2.set_user_option('one', 'c2')
- self.assertEquals('c2', c2._get_user_option('one'))
+ self.assertEquals('c2', c2.get_user_option('one'))
# The first modification is still available until another refresh
# occur
- self.assertEquals('c1', c1._get_user_option('one'))
+ self.assertEquals('c1', c1.get_user_option('one'))
c1.set_user_option('two', 'done')
- self.assertEquals('c2', c1._get_user_option('one'))
+ self.assertEquals('c2', c1.get_user_option('one'))
+
+ def test_writes_are_serialized(self):
+ c1 = self.create_config()
+ 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()
+ 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
+ def c1_set_option():
+ c1.set_user_option('one', 'c1')
+ t1 = threading.Thread(target=c1_set_option)
+ self.addCleanup(t1.join)
+ t1.start()
+ c1_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()
+ c2.set_user_option('one', 'c2')
+ self.assertEquals('c2', c2.get_user_option('one'))
class TestGetUserOptionAs(TestIniConfig):
More information about the bazaar-commits
mailing list