Merge lp:~vila/bzr/lockable-config-files into lp:bzr

[Vincent Ladeuil]

This is the core thread of the loom regarding config files.
This fixes bug #525571 by using a real lock (the previous fix was reducing
the racing window by using a atomic file).

[John A Meinel (jameinel)]

305 + @classmethod
306 + def from_bytes(cls, unicode_bytes):

It seems a little weird to call it 'unicode_bytes' and to have the function be 'from_bytes'. What about 'from_unicode(cls, unicode_content)' ?

I also thought that 'lock_write' would take care of reload(), so that callers couldn't actually get it wrong. (reload whenever the lock transitions from 0 => 1 lockers.)

This seems ok enough to land, but it also seems like it could be improved.

review: Approve

[Vincent Ladeuil (vila)]

> 305 + @classmethod
> 306 + def from_bytes(cls, unicode_bytes):
>
> It seems a little weird to call it 'unicode_bytes' and to have the function be
> 'from_bytes'. What about 'from_unicode(cls, unicode_content)' ?

I just went with what spiv proposed.

I changed it to from_string(str_or_unicode).

>
> I also thought that 'lock_write' would take care of reload(), so that callers
> couldn't actually get it wrong. (reload whenever the lock transitions from 0
> => 1 lockers.)

Hmm, that may be worth considering, but it's outside the scope of this bug.
I've not change the over data model here and forcing the call to reload
may block some use cases. What if I want to inspect the values already known
by the config before reloading from disk (which must happen inside the lock
scope) ?

The constraint to call reload() (which is new, like LockableConfig) shouldn't
be a such burden for new adopters and could even help them understand what
is really happening here (also note that I only needed to add *3* calls and
that reload() usage is more than hinted in LockableConfig docstring).

> This seems ok enough to land, but it also seems like it could be improved.

What can't ? But since this is my third submission for this bug, I think
I've spend enough time on it ;) I intend to keep working in the config area
anyway.

[Andrew Bennetts (spiv)]

Vincent Ladeuil wrote:
> > 305 + @classmethod
> > 306 + def from_bytes(cls, unicode_bytes):
> >
> > It seems a little weird to call it 'unicode_bytes' and to have the function be
> > 'from_bytes'. What about 'from_unicode(cls, unicode_content)' ?
>
> I just went with what spiv proposed.

IIRC, I proposed (or meant to propose) calling that “utf8_bytes”.

-Andrew.

[John A Meinel (jameinel)]

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

On 8/29/2010 7:49 PM, Andrew Bennetts wrote:
> Vincent Ladeuil wrote:
>>> 305 + @classmethod
>>> 306 + def from_bytes(cls, unicode_bytes):
>>>
>>> It seems a little weird to call it 'unicode_bytes' and to have the function be
>>> 'from_bytes'. What about 'from_unicode(cls, unicode_content)' ?
>>
>> I just went with what spiv proposed.
>
> IIRC, I proposed (or meant to propose) calling that “utf8_bytes”.
>
> -Andrew.
>

Right, so my big concern is calling something 'unicode' when it is a
bytes string, or calling it 'bytes' when it is a unicode string. And
calling it 'unicode_bytes' always gets it wrong in one way or another :).

If it can be either than calling the parameter "unicode_or_bytes" though
doing "from_bytes(u'unicode-string')" would also look wrong in practice.

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

iEYEARECAAYFAkx74ZcACgkQJdeBCYSNAAOwMwCgirTL4Dkt7N6ycqZCALusZVcy
QDIAn3eX0ZcLYvrVqinDIDnxmSt+8Eym
=/eoB
-----END PGP SIGNATURE-----

=== modified file 'NEWS'
--- NEWS	2010-08-29 18:07:45 +0000
+++ NEWS	2010-08-29 18:07:46 +0000
@@ -59,6 +59,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
@@ -87,6 +96,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)
 
@@ -167,8 +179,8 @@
 API Changes
 ***********
 
-* Configuration files should now use the ``from_bytes`` constructor the
-  rather than the ``file`` parameter of the ``_get_parser`` method. The
+* ``IniBaseConfig`` objects should now use the ``from_string`` constructor
+  the rather than the ``file`` parameter of the ``_get_parser`` method. The
   later has been deprecated. (Vincent Ladeuil)
 
 * InventoryEntry instances now raise AttributeError if you try to assign
@@ -183,10 +195,6 @@
 * InventoryEntry objects no longer have ``_put_in_tar`` or
   ``_put_on_disk`` methods.  (Andrew Bennetts)
 
-* The ``configIniBaseConfig`` constructor now accepts a ``_content``
-  private parameter for tests that want to provide a config file content.
-  (Vincent Ladeuil)
-
 * The ``get_filename`` parameter in the ``config.IniBaseConfig``
   constructor has been deprecated, use the ``file_name`` parameter instead.
   (Vincent Ladeuil)

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-08-20 09:39:20 +0000
+++ bzrlib/builtins.py	2010-08-29 18:07:46 +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'
--- bzrlib/config.py	2010-08-29 18:07:45 +0000
+++ bzrlib/config.py	2010-08-29 18:07:46 +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,
@@ -359,17 +362,14 @@
         :param file_name: The configuration file path.
         """
         super(IniBasedConfig, self).__init__()
+        self.file_name = file_name
         if symbol_versioning.deprecated_passed(get_filename):
             symbol_versioning.warn(
                 'IniBasedConfig.__init__(get_filename) was deprecated in 2.3.'
                 ' Use file_name instead.',
                 DeprecationWarning,
                 stacklevel=2)
-            if get_filename is None:
-                # Tests use in-memory config files that doesn't need to be
-                # saved on disk
-                self.file_name = None
-            else:
+            if get_filename is not None:
                 self.file_name = get_filename()
         else:
             self.file_name = file_name
@@ -377,16 +377,21 @@
         self._parser = None
 
     @classmethod
-    def from_bytes(cls, unicode_bytes):
-        """Create a config object from bytes.
+    def from_string(cls, str_or_unicode, file_name=None):
+        """Create a config object from a string.
 
-        :param unicode_bytes: A string representing the file content. This will
+        :param str_or_unicode: A string representing the file content. This will
             be utf-8 encoded.
+
+        :param file_name: The configuration file path.
         """
-        conf = cls()
-        conf._content = StringIO(unicode_bytes.encode('utf-8'))
+        conf = cls(file_name=file_name)
+        conf._create_from_string(str_or_unicode)
         return conf
 
+    def _create_from_string(self, str_or_unicode):
+        self._content = StringIO(str_or_unicode.encode('utf-8'))
+
     def _get_parser(self, file=symbol_versioning.DEPRECATED_PARAMETER):
         if self._parser is not None:
             return self._parser
@@ -406,8 +411,17 @@
             self._parser = ConfigObj(co_input, encoding='utf-8')
         except configobj.ConfigObjError, e:
             raise errors.ParseConfigError(e.errors, e.config.filename)
+        # Make sure self.reload() will use the right file name
+        self._parser.filename = self.file_name
         return self._parser
 
+    def reload(self):
+        """Reload the config file from disk."""
+        if self.file_name is None:
+            raise AssertionError('We need a file name to reload the config')
+        if self._parser is not None:
+            self._parser.reload()
+
     def _get_matching_sections(self):
         """Return an ordered list of (section_name, extra_path) pairs.
 
@@ -519,6 +533,8 @@
     def _write_config_file(self):
         if self.file_name is None:
             raise AssertionError('We cannot save, self.file_name is None')
+        conf_dir = os.path.dirname(self.file_name)
+        ensure_config_dir_exists(conf_dir)
         atomic_file = atomicfile.AtomicFile(self.file_name)
         self._get_parser().write(atomic_file)
         atomic_file.commit()
@@ -526,15 +542,82 @@
         osutils.copy_ownership_from_path(self.file_name)
 
 
-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.
+
+    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'
+
+    def __init__(self, file_name):
+        super(LockableConfig, self).__init__(file_name=file_name)
+        self.dir = osutils.dirname(osutils.safe_unicode(self.file_name))
+        self.transport = transport.get_transport(self.dir)
+        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."""
 
     def __init__(self):
         super(GlobalConfig, self).__init__(file_name=config_filename())
 
+    @classmethod
+    def from_string(cls, str_or_unicode):
+        """Create a config object from a string.
+
+        :param str_or_unicode: A string representing the file content. This
+            will be utf-8 encoded.
+        """
+        conf = cls()
+        conf._create_from_string(str_or_unicode)
+        return conf
+
     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')
@@ -546,12 +629,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)
@@ -559,15 +645,12 @@
         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.file_name)
-        ensure_config_dir_exists(conf_dir)
+        self.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):
@@ -581,16 +664,16 @@
         self.location = location
 
     @classmethod
-    def from_bytes(cls, unicode_bytes, location):
-        """Create a config object from bytes.
+    def from_string(cls, str_or_unicode, location):
+        """Create a config object from s string.
 
-        :param unicode_bytes: A string representing the file content. This will
+        :param str_or_unicode: A string representing the file content. This will
             be utf-8 encoded.
 
         :param location: The location url to filter the configuration.
         """
         conf = cls(location)
-        conf._content = StringIO(unicode_bytes.encode('utf-8'))
+        conf._create_from_string(str_or_unicode)
         return conf
 
     def _get_matching_sections(self):
@@ -686,6 +769,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,
@@ -693,10 +777,7 @@
                          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.file_name)
-        ensure_config_dir_exists(conf_dir)
+        self.reload()
         location = self.location
         if location.endswith('/'):
             location = location[:-1]

=== modified file 'bzrlib/plugins/launchpad/test_account.py'
--- bzrlib/plugins/launchpad/test_account.py	2010-08-29 18:07:45 +0000
+++ bzrlib/plugins/launchpad/test_account.py	2010-08-29 18:07:46 +0000
@@ -26,7 +26,7 @@
 class LaunchpadAccountTests(TestCaseInTempDir):
 
     def setup_config(self, text):
-        my_config = config.GlobalConfig.from_bytes(text)
+        my_config = config.GlobalConfig.from_string(text)
         return my_config
 
     def test_get_lp_login_unconfigured(self):

=== modified file 'bzrlib/tests/blackbox/test_break_lock.py'
--- bzrlib/tests/blackbox/test_break_lock.py	2010-07-21 13:32:27 +0000
+++ bzrlib/tests/blackbox/test_break_lock.py	2010-08-29 18:07:46 +0000
@@ -22,8 +22,10 @@
 from bzrlib import (
     branch,
     bzrdir,
+    config,
     errors,
     lockdir,
+    osutils,
     tests,
     )
 
@@ -73,7 +75,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()
@@ -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_commands.py'
--- bzrlib/tests/test_commands.py	2010-08-29 18:07:45 +0000
+++ bzrlib/tests/test_commands.py	2010-08-29 18:07:46 +0000
@@ -95,7 +95,7 @@
 class TestGetAlias(tests.TestCase):
 
     def _get_config(self, config_text):
-        my_config = config.GlobalConfig.from_bytes(config_text)
+        my_config = config.GlobalConfig.from_string(config_text)
         return my_config
 
     def test_simple(self):

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2010-08-29 18:07:45 +0000
+++ bzrlib/tests/test_config.py	2010-08-29 18:07:46 +0000
@@ -19,6 +19,7 @@
 from cStringIO import StringIO
 import os
 import sys
+import threading
 
 #import bzrlib specific imports here
 from bzrlib import (
@@ -39,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]
@@ -130,6 +155,9 @@
         self._calls.append(('keys',))
         return []
 
+    def reload(self):
+        self._calls.append(('reload',))
+
     def write(self, arg):
         self._calls.append(('write',))
 
@@ -367,7 +395,7 @@
 class TestIniConfig(tests.TestCaseInTempDir):
 
     def make_config_parser(self, s):
-        conf = config.IniBasedConfig.from_bytes(s)
+        conf = config.IniBasedConfig.from_string(s)
         return conf, conf._get_parser()
 
 
@@ -377,11 +405,11 @@
         my_config = config.IniBasedConfig()
 
     def test_from_fp(self):
-        my_config = config.IniBasedConfig.from_bytes(sample_config_text)
+        my_config = config.IniBasedConfig.from_string(sample_config_text)
         self.assertIsInstance(my_config._get_parser(), configobj.ConfigObj)
 
     def test_cached(self):
-        my_config = config.IniBasedConfig.from_bytes(sample_config_text)
+        my_config = config.IniBasedConfig.from_string(sample_config_text)
         parser = my_config._get_parser()
         self.failUnless(my_config._get_parser() is parser)
 
@@ -389,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))
 
@@ -409,7 +436,7 @@
 
     def test_get_parser_file_parameter_is_deprecated_(self):
         config_file = StringIO(sample_config_text.encode('utf-8'))
-        conf = config.IniBasedConfig.from_bytes(sample_config_text)
+        conf = config.IniBasedConfig.from_string(sample_config_text)
         conf = self.callDeprecated([
             'IniBasedConfig._get_parser(file=xxx) was deprecated in 2.3.'
             ' Use IniBasedConfig(_content=xxx) instead.'],
@@ -420,6 +447,153 @@
         self.assertRaises(AssertionError, conf._write_config_file)
 
 
+class TestIniBaseConfigOnDisk(tests.TestCaseInTempDir):
+
+    def test_cannot_reload_without_name(self):
+        conf = config.IniBasedConfig.from_string(sample_config_text)
+        self.assertRaises(AssertionError, conf.reload)
+
+    def test_reload_see_new_value(self):
+        c1 = config.IniBasedConfig.from_string('editor=vim\n',
+                                               file_name='./test/conf')
+        c1._write_config_file()
+        c2 = config.IniBasedConfig.from_string('editor=emacs\n',
+                                               file_name='./test/conf')
+        c2._write_config_file()
+        self.assertEqual('vim', c1.get_user_option('editor'))
+        self.assertEqual('emacs', c2.get_user_option('editor'))
+        # Make sure we get the Right value
+        c1.reload()
+        self.assertEqual('emacs', c1.get_user_option('editor'))
+
+
+class TestLockableConfig(tests.TestCaseInTempDir):
+
+    # Set by load_tests
+    config_class = None
+    config_args = None
+    config_section = None
+
+    def setUp(self):
+        super(TestLockableConfig, self).setUp()
+        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.from_string(content, *self.config_args)
+        c.lock_write()
+        c._write_config_file()
+        c.unlock()
+        return c
+
+    def test_simple_read_access(self):
+        self.assertEquals('1', self.config.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'))
+
+    def test_listen_to_the_last_speaker(self):
+        c1 = self.config
+        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'))
+        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.config
+        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'))
+        # 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.config
+        c2 = self.get_existing_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.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):
 
     def test_get_user_option_as_bool(self):
@@ -607,7 +781,7 @@
 class TestGlobalConfigItems(tests.TestCase):
 
     def test_user_id(self):
-        my_config = config.GlobalConfig.from_bytes(sample_config_text)
+        my_config = config.GlobalConfig.from_string(sample_config_text)
         self.assertEqual(u"Erik B\u00e5gfors <[e-mail]>",
                          my_config._get_user_id())
 
@@ -616,11 +790,11 @@
         self.assertEqual(None, my_config._get_user_id())
 
     def test_configured_editor(self):
-        my_config = config.GlobalConfig.from_bytes(sample_config_text)
+        my_config = config.GlobalConfig.from_string(sample_config_text)
         self.assertEqual("vim", my_config.get_editor())
 
     def test_signatures_always(self):
-        my_config = config.GlobalConfig.from_bytes(sample_always_signatures)
+        my_config = config.GlobalConfig.from_string(sample_always_signatures)
         self.assertEqual(config.CHECK_NEVER,
                          my_config.signature_checking())
         self.assertEqual(config.SIGN_ALWAYS,
@@ -628,7 +802,7 @@
         self.assertEqual(True, my_config.signature_needed())
 
     def test_signatures_if_possible(self):
-        my_config = config.GlobalConfig.from_bytes(sample_maybe_signatures)
+        my_config = config.GlobalConfig.from_string(sample_maybe_signatures)
         self.assertEqual(config.CHECK_NEVER,
                          my_config.signature_checking())
         self.assertEqual(config.SIGN_WHEN_REQUIRED,
@@ -636,7 +810,7 @@
         self.assertEqual(False, my_config.signature_needed())
 
     def test_signatures_ignore(self):
-        my_config = config.GlobalConfig.from_bytes(sample_ignore_signatures)
+        my_config = config.GlobalConfig.from_string(sample_ignore_signatures)
         self.assertEqual(config.CHECK_ALWAYS,
                          my_config.signature_checking())
         self.assertEqual(config.SIGN_NEVER,
@@ -644,7 +818,7 @@
         self.assertEqual(False, my_config.signature_needed())
 
     def _get_sample_config(self):
-        my_config = config.GlobalConfig.from_bytes(sample_config_text)
+        my_config = config.GlobalConfig.from_string(sample_config_text)
         return my_config
 
     def test_gpg_signing_command(self):
@@ -993,11 +1167,15 @@
             global_config = sample_config_text
 
         config.ensure_config_dir_exists()
-        my_global_config = config.GlobalConfig.from_bytes(global_config)
+        my_global_config = config.GlobalConfig.from_string(global_config)
+        my_global_config.lock_write()
         my_global_config._write_config_file()
-        my_location_config = config.LocationConfig.from_bytes(
+        my_global_config.unlock()
+        my_location_config = config.LocationConfig.from_string(
             sample_branches_text, my_branch.base)
+        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
@@ -1013,7 +1191,8 @@
                              'converted to use policies.'],
                             self.my_config.set_user_option,
                             'foo', 'bar', store=config.STORE_LOCATION)
-        self.assertEqual([('__contains__', '/a/c'),
+        self.assertEqual([('reload',),
+                          ('__contains__', '/a/c'),
                           ('__contains__', '/a/c/'),
                           ('__setitem__', '/a/c', {}),
                           ('__getitem__', '/a/c'),
@@ -1068,14 +1247,18 @@
                           location_config=None, branch_data_config=None):
         my_branch = FakeBranch(location)
         if global_config is not None:
-            my_global_config = config.GlobalConfig.from_bytes(global_config)
+            my_global_config = config.GlobalConfig.from_string(global_config)
             config.ensure_config_dir_exists()
+            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.from_bytes(
+            my_location_config = config.LocationConfig.from_string(
                 location_config, my_branch.base)
             config.ensure_config_dir_exists()
+            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'] = \

=== modified file 'bzrlib/tests/test_smtp_connection.py'
--- bzrlib/tests/test_smtp_connection.py	2010-08-29 18:07:45 +0000
+++ bzrlib/tests/test_smtp_connection.py	2010-08-29 18:07:46 +0000
@@ -91,7 +91,7 @@
 class TestSMTPConnection(tests.TestCaseInTempDir):
 
     def get_connection(self, text, smtp_factory=None):
-        my_config = config.GlobalConfig.from_bytes(text)
+        my_config = config.GlobalConfig.from_string(text)
         return smtp_connection.SMTPConnection(my_config,
                                               _smtp_factory=smtp_factory)