Merge lp:~vila/bzr/525571-lock-bazaar-conf-files into lp:bzr

[Vincent Ladeuil]

This implements a write lock on LocationConfig and GlobalConfig and add support for them in break-lock
as required for bug #525571.

There is no user nor dev visible change but I'll welcome feedback from plugin authors.

There is a new bzrlib.config.LockableConfig that plugins authors using config files in ~/.bazaar may want to inherit (as LocationConfig and GlobalConfig do now).

I had to do some cleanup in the tests as modifying the model made quite a few of them fail
(congrats to test authors: failing tests are good tests ! :)

So I made a bit more cleanup than strictly necessary (during failure analysis),
my apologies to the reviewers.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/525571-lock-bazaar-conf-files into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #525571 No locking when updating files in ~/.bazaar
> https://bugs.launchpad.net/bugs/525571
>
>
> This implements a write lock on LocationConfig and GlobalConfig and add support for them in break-lock
> as required for bug #525571.
>
> There is no user nor dev visible change but I'll welcome feedback from plugin authors.
>
> There is a new bzrlib.config.LockableConfig that plugins authors using config files in ~/.bazaar may want to inherit (as LocationConfig and GlobalConfig do now).
>
> I had to do some cleanup in the tests as modifying the model made quite a few of them fail
> (congrats to test authors: failing tests are good tests ! :)
>
> So I made a bit more cleanup than strictly necessary (during failure analysis),
> my apologies to the reviewers.
>

As a comment, without having really read the code thoroughly.

How does this handle stuff like 2 branches locking concurrently
locations.conf. I don't know how often we do it internally, though.

I think lots of filesystem locks on the bazaar directory could adversely
affect performance on Windows. IME locking isn't too expensive if you do
it 1 or 2 times. But if you lock and unlock on every attribute that gets
set, then it probably starts to be an issue.

On a Windows host:
  $ TIMEIT -s "b = Branch.open('.')" "b.lock_write(); b.unlock()"
  10.5msec

On an Ubuntu VM on the same machine:
  $ TIMEIT -s "b = Branch.open('.')" "b.lock_write(); b.unlock()"
  1.55msec

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwqMNkACgkQJdeBCYSNAAO7WACfYZOte5LfqA4Ro4J6U/3ZA2Cf
ZhkAoLy3d0+tQjcgx047AYI0sJMiZlfY
=mMJj
-----END PGP SIGNATURE-----

[Robert Collins (lifeless)]

07:04 -!- guilhembi
[~<email address hidden>] has quit
[Quit: Client exiting]
07:11 < lifeless> vila:
07:11 < lifeless> + __doc__ = """\
07:11 < lifeless> + Break a dead lock on a repository, branch,
working directory or config file.
07:11 < lifeless> I'd prefer to see that spelt as
07:11 < lifeless> __doc__ = \
07:11 < lifeless> """Break...
07:12 < lifeless> because having to actually think about what you were
escaping hurt my brain

[Robert Collins (lifeless)]

Meta: This seems like a case where two threads would be nice:
a) fix the tests that are currently a bit gratuitous.
b) make the behaviour change

[Robert Collins (lifeless)]

Ok, actual review stuff:
 the docstring layout is wrong, please nuke the \.

We should check for branches first, not config files, because branch locks are the common case and break-lock doesn't need to be slow.

This change is suspicous:

152 def _write_config_file(self):
153 - f = file(self._get_filename(), "wb")
154 + fname = self._get_filename()
155 + conf_dir = os.path.dirname(fname)
156 + ensure_config_dir_exists(conf_dir)
157 + f = file(fname, "wb")
158 try:
159 - osutils.copy_ownership_from_path(f.name)
160 + osutils.copy_ownership_from_path(fname)
161 self._get_parser().write(f)
162 finally:
163 f.close()
164

It appears to be adding a new stat/mkdir check, at the wrong layer.

missing VWS:

172 + """
173 + lock_name = 'lock'

Ditto here:

181 + def lock_write(self, token=None):
182 + if self.lock is None:
183 + ensure_config_dir_exists(self.dir)
184 + self._lock = lockdir.LockDir(self.transport, self.lock_name)
185 + self._lock.lock_write(token)
186 + return lock.LogicalLockResult(self.unlock)

If the dir doesn't exist, something is wrong - we really shouldn't have gotten this far without it, should we?

Docstrings!!!

I'll review the tests after the core is good.

Uh, you raise NoLockDir, but use it to mean 'A config directory that is not locked' which is very odd. Please consider something that means what you need. Also see my ordering suggestion which may help you not to need this at all.

review: Needs Fixing

[Vincent Ladeuil (vila)]

> Ok, actual review stuff:
> the docstring layout is wrong, please nuke the \.
>
> We should check for branches first, not config files, because branch locks are
> the common case and break-lock doesn't need to be slow.

break_lock() for wt, branch, repo gives no indication about whether it fails or succeeds.
Trying the conf files first was the easiest.

Regarding performance, I think we don't care at all, the user is supposed to first check that
the lock is not hold by a running process (or someone else) which requires seconds in the best case
or occur long after the lock has been created.

>
> This change is suspicous:
>
> 152 def _write_config_file(self):
> 153 - f = file(self._get_filename(), "wb")
> 154 + fname = self._get_filename()
> 155 + conf_dir = os.path.dirname(fname)
> 156 + ensure_config_dir_exists(conf_dir)
> 157 + f = file(fname, "wb")
> 158 try:
> 159 - osutils.copy_ownership_from_path(f.name)
> 160 + osutils.copy_ownership_from_path(fname)
> 161 self._get_parser().write(f)
> 162 finally:
> 163 f.close()
> 164
>
> It appears to be adding a new stat/mkdir check, at the wrong layer.

No adding there, code duplication removal instead, ensure_config_dir_exists() was called anyway.

>
> missing VWS:
>
> 172 + """
> 173 + lock_name = 'lock'

Fixed.
>
>
>
> Ditto here:
>
> 181 + def lock_write(self, token=None):
> 182 + if self.lock is None:
> 183 + ensure_config_dir_exists(self.dir)
> 184 + self._lock = lockdir.LockDir(self.transport,
> self.lock_name)
> 185 + self._lock.lock_write(token)
> 186 + return lock.LogicalLockResult(self.unlock)
>
> If the dir doesn't exist, something is wrong - we really shouldn't have gotten
> this far without it, should we?

When the config file didn't exist, the config dir needs to be created.

> Docstrings!!!

A bit terse but I will add some.

> Uh, you raise NoLockDir, but use it to mean 'A config directory that is not
> locked' which is very odd.
> Please consider something that means what you need.

I need something that means: "Oh, I wanted to break a lock there but there is no lock dir there,
surely I can't break a lock in that case". I fail to see the oddness :-/

[Vincent Ladeuil (vila)]

> As a comment, without having really read the code thoroughly.
>
> How does this handle stuff like 2 branches locking concurrently
> locations.conf. I don't know how often we do it internally, though.

TestLockableConfig.test_writes_are_serialized

>
> I think lots of filesystem locks on the bazaar directory could adversely
> affect performance on Windows. IME locking isn't too expensive if you do
> it 1 or 2 times. But if you lock and unlock on every attribute that gets
> set, then it probably starts to be an issue.

The actual code is not correct, it allows concurrent writers. If higher levels do too many updates of config variables it's a problem in the higher levels, we could imagine holding the updates until the last one, but
this can't addressed here.

Correctness comes before performance, there are many things we can do to address performance but it's way out of scope for this proposal IMHO.

>
> On a Windows host:
> $ TIMEIT -s "b = Branch.open('.')" "b.lock_write(); b.unlock()"
> 10.5msec
>
> On an Ubuntu VM on the same machine:
> $ TIMEIT -s "b = Branch.open('.')" "b.lock_write(); b.unlock()"
> 1.55msec

Thanks, good data point, but still, I haven't seen code doing massive updates of config variables either...

[lp:~vila/bzr/525571-lock-bazaar-conf-files]

5327. By Vincent Ladeuil on 2010-06-30

Revert the lock scope to a sane value.

* bzrlib/tests/test_config.py:
(TestLockableConfig.test_writes_are_serialized)
(TestLockableConfig.test_read_while_writing): Fix the fallouts.

* bzrlib/config.py:
(LockableConfig): Wrong idea, the lock needs to be taken arond the
whole value update call, reducing the lock scope to
_write_config_file exclude the config file re-read.
(GlobalConfig.set_user_option, GlobalConfig.set_alias)
(GlobalConfig.unset_alias, LocationConfig.set_user_option): These
methods needs to be decorated with needs_write_lock to enforce the
design constraints (lock, re-read config, set new value, write
config, unlock).

5328. By Vincent Ladeuil on 2010-06-30

Fix wrong bug number and clarify NEWS entries.

5329. By Vincent Ladeuil on 2010-06-30

Fix docstring.

5330. By Vincent Ladeuil on 2010-06-30

Further clarify NEWS entry.

5331. By Vincent Ladeuil on 2010-06-30

Add a test to help LockableConfig daughter classes identify methods that needs to be decorated.

5332. By Vincent Ladeuil on 2010-06-30

Implement the --conf option for break-lock as per lifeless suggestion.

* bzrlib/errors.py:
(NoLockDir): Useless, deleted.

* bzrlib/config.py:
(LockableConfig.unlock): NoLockDir is useless, break_lock()
silenty succeeds if the directory doesn't exist.

* bzrlib/tests/blackbox/test_break_lock.py:
Tweak the tests.

* bzrlib/builtins.py:
(cmd_break_lock): Fix docstring, add a --conf option to deal with
config files.

5333. By Vincent Ladeuil on 2010-06-30

Final cleanup.

5334. By Vincent Ladeuil on 2010-07-02

Clarify lock scope.

5335. By Vincent Ladeuil on 2010-07-02

Use --config instead of --conf for break-lock.

Unmerged revisions

5335. By Vincent Ladeuil on 2010-07-02

Use --config instead of --conf for break-lock.

5334. By Vincent Ladeuil on 2010-07-02

Clarify lock scope.

5333. By Vincent Ladeuil on 2010-06-30

Final cleanup.

5332. By Vincent Ladeuil on 2010-06-30

Implement the --conf option for break-lock as per lifeless suggestion.

* bzrlib/errors.py:
(NoLockDir): Useless, deleted.

* bzrlib/config.py:
(LockableConfig.unlock): NoLockDir is useless, break_lock()
silenty succeeds if the directory doesn't exist.

* bzrlib/tests/blackbox/test_break_lock.py:
Tweak the tests.

* bzrlib/builtins.py:
(cmd_break_lock): Fix docstring, add a --conf option to deal with
config files.

5331. By Vincent Ladeuil on 2010-06-30

Add a test to help LockableConfig daughter classes identify methods that needs to be decorated.

5330. By Vincent Ladeuil on 2010-06-30

Further clarify NEWS entry.

5329. By Vincent Ladeuil on 2010-06-30

Fix docstring.

5328. By Vincent Ladeuil on 2010-06-30

Fix wrong bug number and clarify NEWS entries.

5327. By Vincent Ladeuil on 2010-06-30

Revert the lock scope to a sane value.

* bzrlib/tests/test_config.py:
(TestLockableConfig.test_writes_are_serialized)
(TestLockableConfig.test_read_while_writing): Fix the fallouts.

* bzrlib/config.py:
(LockableConfig): Wrong idea, the lock needs to be taken arond the
whole value update call, reducing the lock scope to
_write_config_file exclude the config file re-read.
(GlobalConfig.set_user_option, GlobalConfig.set_alias)
(GlobalConfig.unset_alias, LocationConfig.set_user_option): These
methods needs to be decorated with needs_write_lock to enforce the
design constraints (lock, re-read config, set new value, write
config, unlock).

5326. By Vincent Ladeuil on 2010-06-29

Add some comments.

=== modified file 'NEWS'
--- NEWS	2010-06-28 21:45:10 +0000
+++ NEWS	2010-06-30 14:21:34 +0000
@@ -36,6 +36,15 @@
 New Features
 ************
 
+* ``bzr break-lock`` now defines a --config parameter that is required
+  to break locks on config files. (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)
+
 * Support ``--directory`` option for a number of additional commands:
   conflicts, merge-directive, missing, resolve, shelve, switch,
   unshelve, whoami. (Martin von Gagern, #527878)
@@ -62,12 +71,18 @@
   or pull location in locations.conf or branch.conf.
   (Gordon Tyler, #534787)
 
+<<<<<<< TREE
 * ``BzrDir.find_branches`` should ignore branches with missing repositories.
   (Marius Kruger, Robert Collins)
 
 * ``BzrDir.find_bzrdirs`` should ignore dirs that raises PermissionDenied.
   (Marius Kruger, Robert Collins)
 
+=======
+* Configuration files in ``${BZR_HOME}`` are now protected
+  against concurrent writers. (Vincent Ladeuil, #525571)
+
+>>>>>>> MERGE-SOURCE
 * Ensure that wrong path specifications in ``BZR_PLUGINS_AT`` display
   proper error messages. (Vincent Ladeuil, #591215)
 
 
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-06-23 08:19:28 +0000
+++ bzrlib/builtins.py	2010-06-30 14:21:34 +0000
@@ -4830,7 +4830,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.
@@ -4841,17 +4844,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('conf',
+               help='LOCATION is a directory containing configuration files.'),
+        ]
 
-    def run(self, location=None, show=False):
+    def run(self, location=None, conf=False):
         if location is None:
             location = u'.'
-        control, relpath = bzrdir.BzrDir.open_containing(location)
-        try:
-            control.break_lock()
-        except NotImplementedError:
-            pass
+        if conf:
+            c = config.LockableConfig(lambda : location)
+            c.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'
--- bzrlib/config.py	2010-06-02 04:50:35 +0000
+++ bzrlib/config.py	2010-06-30 14:21:34 +0000
@@ -62,9 +62,11 @@
 up=pull
 """
 
+from cStringIO import StringIO
 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 +78,14 @@
 from bzrlib import (
     debug,
     errors,
+    lock,
+    lockdir,
     mail_client,
     osutils,
     registry,
     symbol_versioning,
     trace,
+    transport,
     ui,
     urlutils,
     win32utils,
@@ -356,18 +361,30 @@
         self._get_filename = get_filename
         self._parser = None
 
-    def _get_parser(self, file=None):
+    def _get_parser(self, infile=None):
         if self._parser is not None:
             return self._parser
-        if file is None:
-            input = self._get_filename()
-        else:
-            input = file
+        # Do we have a file name for the config file
+        if self._get_filename is None:
+            fname = None
+        else:
+            fname = self._get_filename()
+        # Do we have a content for the config file ?
+        if infile is None:
+            fname_or_content = fname
+        else:
+            fname_or_content = infile
+        # Build the ConfigObj
+        p = None
         try:
-            self._parser = ConfigObj(input, 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.
@@ -478,27 +495,76 @@
         return self.get_user_option('nickname')
 
     def _write_config_file(self):
-        f = file(self._get_filename(), "wb")
+        fname = self._get_filename()
+        conf_dir = os.path.dirname(fname)
+        ensure_config_dir_exists(conf_dir)
+        f = file(fname, "wb")
         try:
-            osutils.copy_ownership_from_path(f.name)
+            osutils.copy_ownership_from_path(fname)
             self._get_parser().write(f)
         finally:
             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.
+    """
+
+    lock_name = 'lock'
+
+    def __init__(self, get_filename):
+        super(LockableConfig, self).__init__(get_filename)
+        self.dir = osutils.dirname(osutils.safe_unicode(self._get_filename()))
+        self.transport = transport.get_transport(self.dir)
+        self._lock = None
+
+    def lock_write(self, token=None):
+        if self._lock is None:
+            ensure_config_dir_exists(self.dir)
+            self._lock = lockdir.LockDir(self.transport, self.lock_name)
+        self._lock.lock_write(token)
+        return lock.LogicalLockResult(self.unlock)
+
+    def unlock(self):
+        self._lock.unlock()
+
+    def break_lock(self):
+        if self._lock is None:
+            self._lock = lockdir.LockDir(self.transport, self.lock_name)
+        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)
+        fname = self._get_filename()
+        f = StringIO()
+        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)
+
+
+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():
@@ -506,10 +572,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')
@@ -519,15 +587,13 @@
         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:
+            self._parser.reload()
         self._get_parser().setdefault(section, {})[option] = value
         self._write_config_file()
 
 
-class LocationConfig(IniBasedConfig):
+class LocationConfig(LockableConfig):
     """A configuration object that gives the policy for a location."""
 
     def __init__(self, location):
@@ -643,6 +709,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,
@@ -650,10 +717,8 @@
                          STORE_LOCATION_APPENDPATH]:
             raise ValueError('bad storage policy %r for %r' %
                 (store, option))
-        # FIXME: RBC 20051029 This should refresh the parser and also take a
-        # file lock on locations.conf.
-        conf_dir = os.path.dirname(self._get_filename())
-        ensure_config_dir_exists(conf_dir)
+        if self._parser is not None:
+            self._parser.reload()
         location = self.location
         if location.endswith('/'):
             location = location[:-1]
 
=== modified file 'bzrlib/tests/blackbox/test_break_lock.py'
--- bzrlib/tests/blackbox/test_break_lock.py	2010-06-11 07:32:12 +0000
+++ bzrlib/tests/blackbox/test_break_lock.py	2010-06-30 14:21:34 +0000
@@ -18,17 +18,18 @@
 
 import os
 
-import bzrlib
 from bzrlib import (
+    branch,
+    bzrdir,
+    config,
     errors,
     lockdir,
+    osutils,
+    tests,
     )
-from bzrlib.branch import Branch
-from bzrlib.bzrdir import BzrDir
-from bzrlib.tests import TestCaseWithTransport
-
-
-class TestBreakLock(TestCaseWithTransport):
+
+
+class TestBreakLock(tests.TestCaseWithTransport):
 
     # General principal for break-lock: All the elements that might be locked
     # by a bzr operation on PATH, are candidates that break-lock may unlock.
@@ -52,14 +53,14 @@
              'repo/',
              'repo/branch/',
              'checkout/'])
-        bzrlib.bzrdir.BzrDir.create('master-repo').create_repository()
-        self.master_branch = bzrlib.bzrdir.BzrDir.create_branch_convenience(
+        bzrdir.BzrDir.create('master-repo').create_repository()
+        self.master_branch = bzrdir.BzrDir.create_branch_convenience(
             'master-repo/master-branch')
-        bzrlib.bzrdir.BzrDir.create('repo').create_repository()
-        local_branch = bzrlib.bzrdir.BzrDir.create_branch_convenience('repo/branch')
+        bzrdir.BzrDir.create('repo').create_repository()
+        local_branch = bzrdir.BzrDir.create_branch_convenience('repo/branch')
         local_branch.bind(self.master_branch)
-        checkoutdir = bzrlib.bzrdir.BzrDir.create('checkout')
-        bzrlib.branch.BranchReferenceFormat().initialize(
+        checkoutdir = bzrdir.BzrDir.create('checkout')
+        branch.BranchReferenceFormat().initialize(
             checkoutdir, target_branch=local_branch)
         self.wt = checkoutdir.create_workingtree()
 
@@ -73,7 +74,7 @@
         # however, we dont test breaking the working tree because we
         # cannot accurately do so right now: the dirstate lock is held
         # by an os lock, and we need to spawn a separate process to lock it
-        # thne kill -9 it.
+        # then kill -9 it.
         # sketch of test:
         # lock most of the dir:
         self.wt.branch.lock_write()
@@ -82,22 +83,45 @@
         # we need 5 yes's - wt, branch, repo, bound branch, bound repo.
         self.run_bzr('break-lock checkout', stdin="y\ny\ny\ny\n")
         # a new tree instance should be lockable
-        branch = bzrlib.branch.Branch.open('checkout')
-        branch.lock_write()
-        branch.unlock()
+        b = branch.Branch.open('checkout')
+        b.lock_write()
+        b.unlock()
         # and a new instance of the master branch
-        mb = branch.get_master_branch()
+        mb = b.get_master_branch()
         mb.lock_write()
         mb.unlock()
         self.assertRaises(errors.LockBroken, self.wt.unlock)
         self.assertRaises(errors.LockBroken, self.master_branch.unlock)
 
 
-class TestBreakLockOldBranch(TestCaseWithTransport):
+class TestBreakLockOldBranch(tests.TestCaseWithTransport):
 
     def test_break_lock_format_5_bzrdir(self):
         # break lock on a format 5 bzrdir should just return
-        self.make_branch_and_tree('foo', format=bzrlib.bzrdir.BzrDirFormat5())
+        self.make_branch_and_tree('foo', format=bzrdir.BzrDirFormat5())
         out, err = self.run_bzr('break-lock foo')
         self.assertEqual('', out)
         self.assertEqual('', err)
+
+
+class TestConfigBreakLock(tests.TestCaseWithTransport):
+
+    def create_pending_lock(self):
+        def config_file_name():
+            return './my.conf'
+        self.build_tree_contents([(config_file_name(), '[DEFAULT]\none=1\n')])
+        conf = config.LockableConfig(config_file_name)
+        conf.lock_write()
+        return conf
+
+    def test_create_pending_lock(self):
+        conf = self.create_pending_lock()
+        self.addCleanup(conf.unlock)
+        self.assertTrue(conf._lock.is_held)
+
+    def test_break_lock(self):
+        conf = self.create_pending_lock()
+        self.run_bzr('break-lock --conf %s'
+                     % osutils.dirname(conf._get_filename()),
+                     stdin="y\n")
+        self.assertRaises(errors.LockBroken, conf.unlock)
 
=== modified file 'bzrlib/tests/test_commands.py'
--- bzrlib/tests/test_commands.py	2010-05-27 21:16:48 +0000
+++ bzrlib/tests/test_commands.py	2010-06-30 14:21:34 +0000
@@ -97,7 +97,7 @@
     def _get_config(self, config_text):
         my_config = config.GlobalConfig()
         config_file = StringIO(config_text.encode('utf-8'))
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         return my_config
 
     def test_simple(self):
 
=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2010-04-23 07:15:23 +0000
+++ bzrlib/tests/test_config.py	2010-06-30 14:21:34 +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]
@@ -129,6 +156,9 @@
         self._calls.append(('keys',))
         return []
 
+    def reload(self):
+        self._calls.append(('reload',))
+
     def write(self, arg):
         self._calls.append(('write',))
 
@@ -371,7 +401,7 @@
 
     def make_config_parser(self, s):
         conf = config.IniBasedConfig(None)
-        parser = conf._get_parser(file=StringIO(s.encode('utf-8')))
+        parser = conf._get_parser(infile=StringIO(s.encode('utf-8')))
         return conf, parser
 
 
@@ -384,16 +414,144 @@
         config_file = StringIO(sample_config_text.encode('utf-8'))
         my_config = config.IniBasedConfig(None)
         self.failUnless(
-            isinstance(my_config._get_parser(file=config_file),
+            isinstance(my_config._get_parser(infile=config_file),
                         configobj.ConfigObj))
 
     def test_cached(self):
         config_file = StringIO(sample_config_text.encode('utf-8'))
         my_config = config.IniBasedConfig(None)
-        parser = my_config._get_parser(file=config_file)
+        parser = my_config._get_parser(infile=config_file)
         self.failUnless(my_config._get_parser() is parser)
 
 
+class TestLockableConfig(tests.TestCaseInTempDir):
+
+    # 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()
+        text = '[%s]\none=1\ntwo=2\n' % (self.config_section,)
+        self.build_tree_contents([(self.config_file_name(), text)])
+
+    def create_config(self):
+        return self.config_class(*self.config_args)
+
+    def test_simple_read_access(self):
+        c = self.create_config()
+        self.assertEquals('1', c.get_user_option('one'))
+
+    def test_simple_write_access(self):
+        c = self.create_config()
+        c.set_user_option('one', 'one')
+        self.assertEquals('one', c.get_user_option('one'))
+
+    def test_unlocked_config(self):
+        c = self.create_config()
+        self.assertRaises(errors.ObjectNotLocked, c._write_config_file)
+
+    def test_listen_to_the_last_speaker(self):
+        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'))
+        # The second update respect the first 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.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'))
+        # The first modification is still available until another refresh
+        # occur
+        self.assertEquals('c1', c1.get_user_option('one'))
+        c1.set_user_option('two', 'done')
+        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
+        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'))
+
+    def test_read_while_writing(self):
+        c1 = self.create_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.create_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.create_config()
+        self.assertEquals('c1', c3.get_user_option('one'))
+
+
 class TestGetUserOptionAs(TestIniConfig):
 
     def test_get_user_option_as_bool(self):
@@ -583,26 +741,26 @@
     def test_user_id(self):
         config_file = StringIO(sample_config_text.encode('utf-8'))
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         self.assertEqual(u"Erik B\u00e5gfors <[e-mail]>",
                          my_config._get_user_id())
 
     def test_absent_user_id(self):
         config_file = StringIO("")
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         self.assertEqual(None, my_config._get_user_id())
 
     def test_configured_editor(self):
         config_file = StringIO(sample_config_text.encode('utf-8'))
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         self.assertEqual("vim", my_config.get_editor())
 
     def test_signatures_always(self):
         config_file = StringIO(sample_always_signatures)
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         self.assertEqual(config.CHECK_NEVER,
                          my_config.signature_checking())
         self.assertEqual(config.SIGN_ALWAYS,
@@ -612,7 +770,7 @@
     def test_signatures_if_possible(self):
         config_file = StringIO(sample_maybe_signatures)
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         self.assertEqual(config.CHECK_NEVER,
                          my_config.signature_checking())
         self.assertEqual(config.SIGN_WHEN_REQUIRED,
@@ -622,7 +780,7 @@
     def test_signatures_ignore(self):
         config_file = StringIO(sample_ignore_signatures)
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         self.assertEqual(config.CHECK_ALWAYS,
                          my_config.signature_checking())
         self.assertEqual(config.SIGN_NEVER,
@@ -632,7 +790,7 @@
     def _get_sample_config(self):
         config_file = StringIO(sample_config_text.encode('utf-8'))
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         return my_config
 
     def test_gpg_signing_command(self):
@@ -643,7 +801,7 @@
     def _get_empty_config(self):
         config_file = StringIO("")
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         return my_config
 
     def test_gpg_signing_command_unset(self):
@@ -745,10 +903,11 @@
                          [('__init__', config.locations_config_filename(),
                            'utf-8')])
         config.ensure_config_dir_exists()
-        #os.mkdir(config.config_dir())
         f = file(config.branches_config_filename(), 'wb')
-        f.write('')
-        f.close()
+        try:
+            f.write('')
+        finally:
+            f.close()
         oldparserclass = config.ConfigObj
         config.ConfigObj = InstrumentedConfigObj
         try:
@@ -995,10 +1154,15 @@
         else:
             global_file = StringIO(global_config.encode('utf-8'))
         branches_file = StringIO(sample_branches_text.encode('utf-8'))
+        # Make sure the locations config can be reloaded
+        config.ensure_config_dir_exists()
+        f = file(config.locations_config_filename(), 'wb')
+        try:
+            f.write(branches_file.getvalue())
+        finally:
+            f.close
         self.my_config = config.BranchConfig(FakeBranch(location))
-        # Force location config to use specified file
         self.my_location_config = self.my_config._get_location_config()
-        self.my_location_config._get_parser(branches_file)
         # Force global config to use specified file
         self.my_config._get_global_config()._get_parser(global_file)
 
@@ -1007,25 +1171,14 @@
         record = InstrumentedConfigObj("foo")
         self.my_location_config._parser = record
 
-        real_mkdir = os.mkdir
-        self.created = False
-        def checked_mkdir(path, mode=0777):
-            self.log('making directory: %s', path)
-            real_mkdir(path, mode)
-            self.created = True
-
-        os.mkdir = checked_mkdir
-        try:
-            self.callDeprecated(['The recurse option is deprecated as of '
-                                 '0.14.  The section "/a/c" has been '
-                                 'converted to use policies.'],
-                                self.my_config.set_user_option,
-                                'foo', 'bar', store=config.STORE_LOCATION)
-        finally:
-            os.mkdir = real_mkdir
-
-        self.failUnless(self.created, 'Failed to create ~/.bazaar')
-        self.assertEqual([('__contains__', '/a/c'),
+        self.callDeprecated(['The recurse option is deprecated as of '
+                             '0.14.  The section "/a/c" has been '
+                             'converted to use policies.'],
+                            self.my_config.set_user_option,
+                            'foo', 'bar', store=config.STORE_LOCATION)
+
+        self.assertEqual([('reload',),
+                          ('__contains__', '/a/c'),
                           ('__contains__', '/a/c/'),
                           ('__setitem__', '/a/c', {}),
                           ('__getitem__', '/a/c'),
@@ -1083,10 +1236,13 @@
         if global_config is not None:
             global_file = StringIO(global_config.encode('utf-8'))
             my_config._get_global_config()._get_parser(global_file)
-        self.my_location_config = my_config._get_location_config()
+        lconf = my_config._get_location_config()
         if location_config is not None:
             location_file = StringIO(location_config.encode('utf-8'))
-            self.my_location_config._get_parser(location_file)
+            lconf._get_parser(location_file)
+            # Make sure the config can be reloaded
+            lconf._parser.filename = config.locations_config_filename()
+        self.my_location_config = lconf
         if branch_data_config is not None:
             my_config.branch.control_files.files['branch.conf'] = \
                 branch_data_config