Rev 5324: Fix fallouts. in file:///home/vila/src/bzr/bugs/525571-lock-bazaar-conf-files/
Vincent Ladeuil
v.ladeuil+lp at free.fr
Tue Jun 29 10:19:11 BST 2010
At file:///home/vila/src/bzr/bugs/525571-lock-bazaar-conf-files/
------------------------------------------------------------
revno: 5324
revision-id: v.ladeuil+lp at free.fr-20100629091911-uuzfksmfm0z9oxtq
parent: v.ladeuil+lp at free.fr-20100629073659-z7cmys2po4ztabr3
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: 525571-lock-bazaar-conf-files
timestamp: Tue 2010-06-29 11:19:11 +0200
message:
Fix fallouts.
* bzrlib/tests/test_config.py:
(TestLockableConfig.test_writes_are_serialized): Fix a race
condition introduced by the refactoring and override the _atomic
method.
* bzrlib/config.py:
(IniBasedConfig._get_parser): Once the config file has been
parsed, the filename (if available) should be set so that reload()
can be used.
(LockableConfig): Simplify the implementation.
(LockableConfig._write_config_file_atomic): For test purposes, we
need to separate the method receiving the needs_write_lock
decorator from the method that actually implements the atomic
write.
-------------- next part --------------
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py 2010-06-29 07:36:59 +0000
+++ b/bzrlib/config.py 2010-06-29 09:19:11 +0000
@@ -78,6 +78,7 @@
from bzrlib import (
debug,
errors,
+ lock,
lockdir,
mail_client,
osutils,
@@ -363,15 +364,24 @@
def _get_parser(self, infile=None):
if self._parser is not None:
return self._parser
+ if self._get_filename is None:
+ fname = None
+ else:
+ fname = self._get_filename()
if infile is None:
- fname_or_content = self._get_filename()
+ fname_or_content = fname
else:
fname_or_content = infile
+ p = None
try:
- self._parser = ConfigObj(fname_or_content, encoding='utf-8')
+ p = ConfigObj(fname_or_content, encoding='utf-8')
except configobj.ConfigObjError, e:
raise errors.ParseConfigError(e.errors, e.config.filename)
- return self._parser
+ if p is not None and fname is not None:
+ # Make sure p.reload() will use the right file name
+ p.filename = fname
+ self._parser = p
+ return p
def _get_matching_sections(self):
"""Return an ordered list of (section_name, extra_path) pairs.
@@ -500,27 +510,29 @@
serialized.
"""
- def __init__(self, get_filename):
- super(LockableConfig, self).__init__(get_filename)
- self.transport = transport.get_transport(config_dir())
+ def lock_write(self, token=None):
+ conf_dir = os.path.dirname(self._get_filename())
+ ensure_config_dir_exists(conf_dir)
+ self.transport = transport.get_transport(conf_dir)
self._lock = lockdir.LockDir(self.transport, 'lock')
-
- def lock_write(self, token=None):
- ensure_config_dir_exists(config_dir())
- return self._lock.lock_write(token)
+ self._lock.lock_write(token)
+ return lock.LogicalLockResult(self.unlock)
def unlock(self):
self._lock.unlock()
+ @needs_write_lock
def _write_config_file(self):
+ self._write_config_file_atomic()
+
+ def _write_config_file_atomic(self):
fname = self._get_filename()
- conf_dir = os.path.dirname(fname)
- # the transport API won't take the windows special case into account
- # here (see ensure_config_dir_exists).
- ensure_config_dir_exists(conf_dir)
f = StringIO()
- self._get_parser().write(f)
+ p = self._get_parser()
+ p.write(f)
self.transport.put_bytes(os.path.basename(fname), f.getvalue())
+ # Make sure p.reload() will use the right file name
+ p.filename = fname
osutils.copy_ownership_from_path(fname)
@@ -530,7 +542,6 @@
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')
@@ -680,7 +691,6 @@
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,
=== modified file 'bzrlib/tests/test_config.py'
--- a/bzrlib/tests/test_config.py 2010-06-29 07:36:59 +0000
+++ b/bzrlib/tests/test_config.py 2010-06-29 09:19:11 +0000
@@ -479,28 +479,34 @@
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()
+ before_writing = threading.Event()
+ after_writing = threading.Event()
+ writing_done = threading.Event()
+ c1_orig = c1._write_config_file_atomic
+ def c1_write_config_file_atomic():
+ 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
+ after_writing.wait()
+ c1._write_config_file_atomic = c1_write_config_file_atomic
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(writing_done.set)
t1.start()
- c1_before_writing.wait()
+ 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()
+ after_writing.set()
+ writing_done.wait()
c2.set_user_option('one', 'c2')
self.assertEquals('c2', c2.get_user_option('one'))
@@ -510,15 +516,15 @@
ready_to_write = threading.Event()
do_writing = threading.Event()
writing_done = threading.Event()
- c1_orig = c1._write_config_file
- def c1_write_config_file():
+ c1_orig = c1._write_config_file_atomic
+ def c1_write_config_file_atomic():
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
+ c1._write_config_file_atomic = c1_write_config_file_atomic
def c1_set_option():
c1.set_user_option('one', 'c1')
t1 = threading.Thread(target=c1_set_option)
More information about the bazaar-commits
mailing list