Rev 5368: Merge lockable-config-files into remove-gratuitous-ensure-config-dir-exist-calls resolving conflicts in file:///home/vila/src/bzr/bugs/525571-lock-bazaar-conf-files/
Vincent Ladeuil
v.ladeuil+lp at free.fr
Mon Aug 23 17:38:45 BST 2010
At file:///home/vila/src/bzr/bugs/525571-lock-bazaar-conf-files/
------------------------------------------------------------
revno: 5368 [merge]
revision-id: v.ladeuil+lp at free.fr-20100823163845-vp2ip6burcfvm1cs
parent: v.ladeuil+lp at free.fr-20100823093422-h6el53jmv5tkxbxg
parent: v.ladeuil+lp at free.fr-20100823163633-zb20v3aewn2053xw
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: remove-gratuitous-ensure-config-dir-exist-calls
timestamp: Mon 2010-08-23 18:38:45 +0200
message:
Merge lockable-config-files into remove-gratuitous-ensure-config-dir-exist-calls resolving conflicts
modified:
NEWS NEWS-20050323055033-4e00b5db738777ff
bzrlib/builtins.py builtins.py-20050830033751-fc01482b9ca23183
bzrlib/config.py config.py-20051011043216-070c74f4e9e338e8
bzrlib/tests/blackbox/test_break_lock.py test_break_lock.py-20060303014503-a90e07d38d042d1d
bzrlib/tests/test_config.py testconfig.py-20051011041908-742d0c15d8d8c8eb
-------------- next part --------------
=== modified file 'NEWS'
--- a/NEWS 2010-08-23 09:34:22 +0000
+++ b/NEWS 2010-08-23 16:38:45 +0000
@@ -36,6 +36,15 @@
New Features
************
+* ``bzr break-lock --config [location]`` can now break config files
+ locks. (Vincent Ladeuil, #525571)
+
+* ``bzrlib.config.LockableConfig`` is a base class for config files that
+ needs to be protected against multiple writers. All methods that
+ change a configuration variable value must be decorated with
+ @needs_write_lock (set_option() for example).
+ (Vincent Ladeuil, #525571)
+
* The ``lp:`` prefix will now use your known username (from
``bzr launchpad-login``) to expand ``~`` to your username. For example:
``bzr launchpad-login user && bzr push lp:~/project/branch`` will now
@@ -64,6 +73,9 @@
* CommitBuilder now uses the committer instead of _config.username to generate
the revision-id. (Aaron Bentley, #614404)
+* Configuration files in ``${BZR_HOME}`` are now protected against
+ concurrent writers by using a lock. (Vincent Ladeuil, #525571)
+
* Cope with Microsoft FTP Server and VSFTPd that return reply '250
Directory created' when mkdir succeeds. (Martin Pool, #224373)
=== modified file 'bzrlib/builtins.py'
--- a/bzrlib/builtins.py 2010-08-20 09:39:20 +0000
+++ b/bzrlib/builtins.py 2010-08-23 16:34:38 +0000
@@ -32,7 +32,7 @@
bzrdir,
directory_service,
delta,
- config,
+ config as _mod_config,
errors,
globbing,
hooks,
@@ -3322,7 +3322,7 @@
try:
c = Branch.open_containing(u'.')[0].get_config()
except errors.NotBranchError:
- c = config.GlobalConfig()
+ c = _mod_config.GlobalConfig()
else:
c = Branch.open(directory).get_config()
if email:
@@ -3333,7 +3333,7 @@
# display a warning if an email address isn't included in the given name.
try:
- config.extract_email_address(name)
+ _mod_config.extract_email_address(name)
except errors.NoEmailInUsername, e:
warning('"%s" does not seem to contain an email address. '
'This is allowed, but not recommended.', name)
@@ -3345,7 +3345,7 @@
else:
c = Branch.open(directory).get_config()
else:
- c = config.GlobalConfig()
+ c = _mod_config.GlobalConfig()
c.set_user_option('email', name)
@@ -3418,13 +3418,13 @@
'bzr alias --remove expects an alias to remove.')
# If alias is not found, print something like:
# unalias: foo: not found
- c = config.GlobalConfig()
+ c = _mod_config.GlobalConfig()
c.unset_alias(alias_name)
@display_command
def print_aliases(self):
"""Print out the defined aliases in a similar format to bash."""
- aliases = config.GlobalConfig().get_aliases()
+ aliases = _mod_config.GlobalConfig().get_aliases()
for key, value in sorted(aliases.iteritems()):
self.outf.write('bzr alias %s="%s"\n' % (key, value))
@@ -3440,7 +3440,7 @@
def set_alias(self, alias_name, alias_command):
"""Save the alias in the global config."""
- c = config.GlobalConfig()
+ c = _mod_config.GlobalConfig()
c.set_alias(alias_name, alias_command)
@@ -4807,7 +4807,10 @@
class cmd_break_lock(Command):
- __doc__ = """Break a dead lock on a repository, branch or working directory.
+ __doc__ = """Break a dead lock.
+
+ This command breaks a lock on a repository, branch, working directory or
+ config file.
CAUTION: Locks should only be broken when you are sure that the process
holding the lock has been stopped.
@@ -4818,17 +4821,27 @@
:Examples:
bzr break-lock
bzr break-lock bzr+ssh://example.com/bzr/foo
+ bzr break-lock --conf ~/.bazaar
"""
+
takes_args = ['location?']
+ takes_options = [
+ Option('config',
+ help='LOCATION is the directory where the config lock is.'),
+ ]
- def run(self, location=None, show=False):
+ def run(self, location=None, config=False):
if location is None:
location = u'.'
- control, relpath = bzrdir.BzrDir.open_containing(location)
- try:
- control.break_lock()
- except NotImplementedError:
- pass
+ if config:
+ conf = _mod_config.LockableConfig(file_name=location)
+ conf.break_lock()
+ else:
+ control, relpath = bzrdir.BzrDir.open_containing(location)
+ try:
+ control.break_lock()
+ except NotImplementedError:
+ pass
class cmd_wait_until_signalled(Command):
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py 2010-08-23 09:34:22 +0000
+++ b/bzrlib/config.py 2010-08-23 16:38:45 +0000
@@ -542,6 +542,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'
@@ -554,12 +572,26 @@
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 break_lock(self):
+ self._lock.break_lock()
+
+ 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."""
@@ -583,12 +615,15 @@
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."""
+ self.reload()
aliases = self._get_parser().get('ALIASES')
if not aliases or alias_name not in aliases:
raise errors.NoSuchAlias(alias_name)
@@ -601,7 +636,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, _save=False):
@@ -708,6 +743,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,
@@ -716,8 +752,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.
- parser = self._get_parser()
location = self.location
if location.endswith('/'):
location = location[:-1]
=== modified file 'bzrlib/tests/blackbox/test_break_lock.py'
--- a/bzrlib/tests/blackbox/test_break_lock.py 2010-07-22 08:00:02 +0000
+++ b/bzrlib/tests/blackbox/test_break_lock.py 2010-08-23 16:34:38 +0000
@@ -22,8 +22,10 @@
from bzrlib import (
branch,
bzrdir,
+ config,
errors,
lockdir,
+ osutils,
tests,
)
@@ -101,3 +103,24 @@
out, err = self.run_bzr('break-lock foo')
self.assertEqual('', out)
self.assertEqual('', err)
+
+class TestConfigBreakLock(tests.TestCaseWithTransport):
+
+ def setUp(self):
+ super(TestConfigBreakLock, self).setUp()
+ self.config_file_name = './my.conf'
+ self.build_tree_contents([(self.config_file_name,
+ '[DEFAULT]\none=1\n')])
+ self.config = config.LockableConfig(file_name=self.config_file_name)
+ self.config.lock_write()
+
+ def test_create_pending_lock(self):
+ self.addCleanup(self.config.unlock)
+ self.assertTrue(self.config._lock.is_held)
+
+ def test_break_lock(self):
+ self.run_bzr('break-lock --config %s'
+ % osutils.dirname(self.config_file_name),
+ stdin="y\n")
+ self.assertRaises(errors.LockBroken, self.config.unlock)
+
=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py 2010-08-23 09:34:22 +0000
+++ b/bzrlib/tests/test_config.py 2010-08-23 16:38:45 +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]
@@ -393,14 +417,13 @@
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
- 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))
@@ -454,16 +477,24 @@
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.lock_write()
c._write_config_file()
+ c.unlock()
return c
def test_simple_read_access(self):
@@ -475,7 +506,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'))
@@ -487,7 +518,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'))
@@ -498,8 +529,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()
@@ -533,6 +564,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):
@@ -1101,10 +1169,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
@@ -1177,11 +1249,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'] = \
More information about the bazaar-commits
mailing list