Merge lp:~vila/bzr/525571-minimal-atomic-write-bazaar-conf-files-2.1 into lp:bzr/2.1

[Vincent Ladeuil]

Minimal fix for bug #525571 to ease backport to launchpad.

This is a minimal patch to address bug #525571 (concurrent config writers) without any attempt to cleanup anything ;)

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/525571-minimal-lock-bazaar-conf-files-2.1 into lp:bzr/2.1.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This is a minimal patch to address bug #525571 (concurrent config writers) without any attempt to cleanup anything ;)
>
>

1) If we know we are accessing a local file (since you are using open()
directly), then we could just use AtomicFile directly.

2) I'm a bit concerned about the need for
safe_unicode(self._get_filename()).

3) You can do

dirname, basename = osutils.split(fname)

rather than doing it 2x.

Personally, I'd really rather see AtomicFile than Transport for this.

Specifically, if fname was ever Unicode, this would break. As Transport
expects the 'relpath' to be a URL, and you are handing it a Unicode
string. Oh, and it would probably break if fname had any funny
characters like % in it.

 review: needsfixing

John
=:->

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

iEYEARECAAYFAkwt9DoACgkQJdeBCYSNAAOE7ACgzcbdLqynPzKMStWmEZkVgOtA
2jYAn0BNItWLFxQCecwo/NHh2x7+jOCG
=EmU/
-----END PGP SIGNATURE-----

review: Needs Fixing

[Vincent Ladeuil (vila)]

> 1) If we know we are accessing a local file (since you are using open()
> directly), then we could just use AtomicFile directly.

Done.

>
> 2) I'm a bit concerned about the need for
> safe_unicode(self._get_filename()).

This was triggered in my cleanup branch by a test in a unicode home dir, Ill look into it again there.

>
> 3) You can do
>
> dirname, basename = osutils.split(fname)
>
> rather than doing it 2x.

1) I was aiming minimal, 2) different encodings were needed.

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-06-16 12:12:56 +0000
+++ NEWS	2010-07-02 16:10:46 +0000
@@ -20,6 +20,10 @@
 Bug Fixes
 *********
 
+* Configuration files in ``${BZR_HOME}`` are now written in an atomic
+  way which should help avoid problems with concurrent writers.
+  (Vincent Ladeuil, #525571)
+
 * Raise ValueError instead of a string exception.
   (John Arbash Meinel, #586926)
 

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2010-02-17 17:11:16 +0000
+++ bzrlib/config.py	2010-07-02 16:10:46 +0000
@@ -74,6 +74,7 @@
 
 import bzrlib
 from bzrlib import (
+    atomicfile,
     debug,
     errors,
     mail_client,
@@ -510,9 +511,10 @@
         self._write_config_file()
 
     def _write_config_file(self):
-        f = open(self._get_filename(), 'wb')
-        self._get_parser().write(f)
-        f.close()
+        atomic_file = atomicfile.AtomicFile(self._get_filename())
+        self._get_parser().write(atomic_file)
+        atomic_file.commit()
+        atomic_file.close()
 
 
 class LocationConfig(IniBasedConfig):
@@ -653,7 +655,10 @@
         self._get_parser()[location][option]=value
         # the allowed values of store match the config policies
         self._set_option_policy(location, option, store)
-        self._get_parser().write(file(self._get_filename(), 'wb'))
+        atomic_file = atomicfile.AtomicFile(self._get_filename())
+        self._get_parser().write(atomic_file)
+        atomic_file.commit()
+        atomic_file.close()
 
 
 class BranchConfig(Config):