No locking when updating files in ~/.bazaar

I'm not sure what the best way is to fix this. I guess we'll have to start locking ~/.bazaar/subversion.conf.

This is one of the few things that requires manual intervention in the import service these days, so can I request a bump in priority?

lp:gcc lp:gcc/4.5 lp:gcc/4.4 lp:gcc/4.3 are affected.

There quite a few failures with 4.3. and 4.4 getting stuck for 12 hours on incremental import =(

http://launchpadlibrarian.net/46133501/mingw-w64-gcc-4.4.log

See bzrlib/config.py:514

        # FIXME: RBC 20051029 This should refresh the parser and also take a
        # file lock on bazaar.conf.

Jelmer Vernooij is it currently broken?

  File "/home/pqm/for_rollouts/production/eggs/bzr-2.1.0-py2.5-linux-x86_64.egg/bzrlib/config.py", line 360, in _get_parser
bzrlib.errors.ParseConfigError: Error(s) parsing config file /home/importd/.bazaar/subversion.conf:
Invalid line at line "575".
Duplicate keyword name at line 576.
Duplicate keyword name at line 577

that's on lp:gcc branches.....

bzrlib.errors.ParseConfigError: Error(s) parsing config file /home/importd/.bazaar/subversion.conf:
Invalid line at line "398".
Import failed:
Traceback (most recent call last):

again gcc...

I don't know if this is the same bug, but there seems to be another concurrency-related issue showing up in LPs svn imports. the 'zope.password' import is now failing[1] with the tracceback:

2010-05-10 17:06:46 INFO Starting job.
2010-05-10 17:06:46 INFO Getting exising bzr branch from central store.
2010-05-10 17:06:57 INFO 35 bytes transferred |
2010-05-10 17:06:59 INFO
2010-05-10 17:06:59 WARNING Upgrade to svn 1.5 or higher for faster retrieving of revision properties.
Traceback (most recent call last):
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/scripts/code-import-worker.py", line 82, in <module>
    sys.exit(script.main())
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/scripts/code-import-worker.py", line 77, in main
    return import_worker.run()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/lib/lp/codehosting/codeimport/worker.py", line 437, in run
    return self._doImport()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/lib/lp/codehosting/codeimport/worker.py", line 571, in _doImport
    foreign_branch_tip = foreign_branch.last_revision()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/branch.py", line 501, in last_revision
    last_revmeta, mapping = self.last_revmeta()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/branch.py", line 256, in last_revmeta
    for revmeta, mapping in self._revision_meta_history():
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/util.py", line 142, in next
    return self._next()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/util.py", line 124, in _next
    ret = self._iterator.next()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/repository.py", line 825, in _iter_reverse_revmeta_mapping_history
    (mapping, lhs_mapping) = revmeta.get_appropriate_mappings(mapping)
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/revmeta.py", line 344, in get_appropriate_mappings
    original = self.get_original_mapping()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/revmeta.py", line 843, in get_original_mapping
    self._original_mapping = self.base.get_original_mapping()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/revmeta.py", line 372, in get_original_mapping
    revprops_acceptable=revprops_acceptable)
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/revmeta.py", line 683, in _import_from_props
    if revprops_acceptable(revprops):
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/revmeta.py", line 367, in revprops_acceptable
    return revprops.get(SVN_REVPROP_BZR_ROOT) == self.branch_path
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/util.py", line 67, in get
    self._ensure_init()
  File "/srv/importd.launchpad.net/production/launchpad-rev-9329/optionalbzrplugins/svn/util.py", l...

On 10 May 2010 18:12, Tres Seaver <email address hidden> wrote:
> I don't know if this is the same bug, but there seems to be another
> concurrency-related issue showing up in LPs svn imports.  the
> 'zope.password' import is now failing[1] with the tracceback:

Yeap gcc imports were failing with that as well before subversion.conf
was adjusted to include all imports from that svn root id.

Now it "sometimes" fails with a different error as mentioned above =)

@Michael yes, you can. The fix is in three parts:
 a) make sure that we don't have any code paths that set the entire conf without reading it immediately and none that edit the file in place.
 b) add a LockDir around the read-edit-write component of changing the config file (possibly around all edits to ~/.bazaar in fact). I suggest ~/.bazaar/lock as the path of the lock.
 c) We'll need some docs to say that there is a lock being taken, and how to release it if bzr is kill -9'd while holding it. This may mean making 'break-lock' accept the url of a lock as well as the current url of a control dir. Or something.

Got another corruption of .bazaar/subversion.conf on pear just now

On Fri, Jun 4, 2010 at 10:02 AM, Jelmer Vernooij <email address hidden> wrote:
> ** Summary changed:
>
> - bzr svn-import fails when invoked concurrently
> + bzrlib.config.IniBasedConfig doesn't lock

I don't think thats really the problem, the problem is that when we
use that for locations.conf, that we don't lock around it. Fixing this
by making IniBasedConfig lock would be a mistake - it would knock on
into nearly all of our configs - branches, repos, etc.

Fair enough - updated.

Can you please stop changing the title every hour? xD

On Fri, Jun 4, 2010 at 2:02 PM, Jelmer Vernooij <email address hidden> wrote:

> ** Summary changed:
>
> - LocationsConfig doesn't lock around bzrlib.config.IniBasedConfig
> + No locking when updating files in ~/.bazaar
>
> --
> No locking when updating files in ~/.bazaar
> https://bugs.launchpad.net/bugs/525571
> You received this bug notification because you are a direct subscriber
> of a duplicate bug.
>

--
marcos garcía // <email address hidden>

Can anyone involved in this bug attached a corrupted subversion.conf file please ?

Thanks jelmer, now does someone have a corrupted one ?

Vincent, the corruption isn't a damaged file, its a file without
content that another concurrent writer added.

On Mon, 28 Jun 2010 13:01:10 you wrote:
> Vincent, the corruption isn't a damaged file, its a file without
> content that another concurrent writer added.

Actually I think it is a damaged file as the config reader cannot read the new
one.

Observations have shown it intermingles the lines, so instead of having:

[heading1]
value=some value
[heading2]
value=some other value

we end up with

[heading1]
[heading2]
value=some value
value=some other value

and the reader blows up with duplicate keys.

Thanks for clarifying, thats really interesting - and even more worrying :)

-Rob

>>>>> "Tim" == Tim Penhey <email address hidden> writes:

    Tim> On Mon, 28 Jun 2010 13:01:10 you wrote:
    >> Vincent, the corruption isn't a damaged file, its a file without
    >> content that another concurrent writer added.

Doesn't match the case reported in the bug description.

Well, if by 'file without content' you mean an empty section (aka only
the section header), it matches, but there is no code in configobj to
back this hypothesis (AFAICS, and if it was that may be in a bug in
bzr-svn, but I dont think so).

    Tim> Actually I think it is a damaged file as the config reader
    Tim> cannot read the new one.

More like it.

    Tim> Observations have shown it intermingles the lines, so instead
    Tim> of having:

I like the intermingled lines cause, thanks for giving weight to it :)

    Tim> [heading1]
    Tim> value=some value
    Tim> [heading2]
    Tim> value=some other value

    Tim> we end up with

    Tim> [heading1]
    Tim> [heading2]
    Tim> value=some value
    Tim> value=some other value

    Tim> and the reader blows up with duplicate keys.

But this example still presents complete sections which the reported one
doesn't:

[261e6e09-138e-40a9-b6e2-6d67e07423dd]
locations = file:///tmp/foo
istedmatrix.com/svn/Twisted
guessed-layout = trunk0
[261e6e09-138e-40a9-b6e2-6d67e07423dd]

Here, only the section header is written without any content to put
into (and *that* doesn't make sense).

One can imagine the first process writing its section, the second one
writing its own (explaining that the same section is created twice) and
then the first process truncating the file while closing it and doing so
losing part of the second process section... and finally a third process
crashing while trying to read the result

A bit far fetched though...

Especally when the python code to write such a file is:

            h = open(self.filename, 'wb')
            h.write(output)
            h.close()

'wb' line-buffered ? Huh ? Anyone with a live chicken to spare ?

Anyway, the fix I'm working on should cover this case, but still, seeing
a corrupted file may help reaching a better understanding.

Died again. Sample:

importd@pear:~/.bazaar$ tail subversion.conf.SAVE
locations = https://pidgin-birthday.svn.sourceforge.net/svnroot/pidgin-birthday
[a60eed84-fc3d-0410-a09c-0d1a7a6707c9]
locations = http://creoleparser.googlecode.com/svn
guessed-layout = trunk0
[cda61777-01e9-0310-a592-d414129be87e]
locations = svn://svn.tartarus.org/sgt
guessed-layout = root
52-e9e23c5afa6a]
locations = svn://svn.alioth.debian.org/svn/pkg-voip
guessed-layout = trunk1

attached a copy of the corrupted subversion.conf file; vila/rob wanting to check for power of 2 issues.

@Steve: thanks.

I checked, but that's inconclusive. It doesn't invalidate the fix either anyway.

A minimal fix that should work around the problem will be part of bzr-2.1.3 when it become available.
A more complete fix is still discussed for bzr.dev.

Fixed in stable r11271 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/11271>.

Fixed in stable r11280 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/11280>.

This was tested by mwhudson on staging.

A more robust fix has landed in bzr.dev, it implements a real lock for config files in ${BZR_HOME}: bazaar.conf and locations.conf.
This is done with a LockableConfig class that can now be used by bzr-svn if needed.

Dear ~ubuntu-sru, we'd like to get this into lucid-updates.
Per <https://wiki.ubuntu.com/StableReleaseUpdates/MicroReleaseExceptions>, we have an exception to take all of 2.1.3, conditional on running the tests during the package build and on running the tests in 2.1.3.
Some of those will fail in 2.1.3, but they should be fixed in 2.1.4.
For now I propose we build the package, then check there are only the expected failures in the package.

Reopening Maverick task. As per Vincent this is fixed in 2.2.1, but maverick has 2.2.0.

Accepted bzr into maverick-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

This bug was fixed in the package bzr - 2.2.1-0ubuntu1

---------------
bzr (2.2.1-0ubuntu1) maverick-proposed; urgency=low

  * New upstream release.
   + Fix regression with upgrading poor-root to rich-root formats. LP: #636930
   + Files in ~/.bazaar are now locked whilst being updated automatically.
     LP: #525571
 -- Max Bowsher <email address hidden> Thu, 30 Sep 2010 09:57:15 +0100

Natty has 2.3.0 beta 2, assuming that it's fixed there.

Accepted bzr into lucid-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

This bug was fixed in the package bzr - 2.1.4-0ubuntu1

---------------
bzr (2.1.4-0ubuntu1) lucid-proposed; urgency=low

  * Update watch file to use 2.1 series.
  * New upstream release.
   + Fix file descriptors leaks in dirstate compiled extension. LP: #583486
   + Refuse to stack on repositories in incompatible formats. LP: #562380
   + Don't delete nested trees/repos. LP: #572098
   + Fix 'bzr switch' crash when a 'ConfigurableFileMerger' is used. LP: #559436
   + Fix compatibility with older smart servers. LP: #528041
   + Fix symlinks addition. LP: #192859
   + Properly unversion children of unversioned directories. LP: #494221
   + Lock configuration files in '~/.bazaar' for updates. LP: #525571
   + Fix 'bzr commit <symlink>'. LP: #128562
   + Fix `lp:` urls when behind an http proxy. LP: #558343
   + Stop using edge.launchpad.net. LP: #583667
 -- Max Bowsher <email address hidden> Tue, 17 May 2011 09:54:17 +0100