No locking when updating files in ~/.bazaar

Bug #525571 reported by Jean-Paul Calderone
72
This bug affects 9 people
Affects Status Importance Assigned to Milestone
Bazaar
High
Vincent Ladeuil
Bazaar Subversion Plugin
High
Vincent Ladeuil
Launchpad itself
High
Jelmer Vernooij
bzr (Ubuntu)
Undecided
Unassigned
Lucid
Undecided
Unassigned
Maverick
Undecided
Unassigned

Bug Description

exarkun@boson:/tmp$ svnadmin create foo
exarkun@boson:/tmp$ bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr
Initialising Subversion metadata cache in /home/exarkun/.cache/bazaar/svn/261e6e09-138e-40a9-b6e2-6d67e07423dd.
Using repository layout: root
Use 'bzr checkout' to create a working tree in the newly created branches.
exarkun@boson:/tmp$ bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr
Using repository layout: root
Use 'bzr checkout' to create a working tree in the newly created branches.
exarkun@boson:/tmp$ bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr
Using repository layout: root
Use 'bzr checkout' to create a working tree in the newly created branches.
exarkun@boson:/tmp$ ls foo-bzr/
exarkun@boson:/tmp$ bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr &bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr &bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr
[1] 19190
[2] 19191
Using repository layout: root
Using repository layout: root
bzr: ERROR: Error(s) parsing config file /home/exarkun/.bazaar/subversion.conf:
Invalid line at line "3".
Duplicate section name at line 5.
Duplicate keyword name at line 6.
Use 'bzr checkout' to create a working tree in the newly created branches.
Use 'bzr checkout' to create a working tree in the newly created branches.
[1]- Exit 3 bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr
[2]+ Done bzr svn-import --incremental -v /tmp/foo /tmp/foo-bzr
exarkun@boson:/tmp$ cat /home/exarkun/.bazaar/subversion.conf
[261e6e09-138e-40a9-b6e2-6d67e07423dd]
locations = file:///tmp/foo
istedmatrix.com/svn/Twisted
guessed-layout = trunk0
[261e6e09-138e-40a9-b6e2-6d67e07423dd]

Related branches

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

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

Changed in bzr-svn:
status: New → Triaged
importance: Undecided → Medium
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

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?

Changed in launchpad-code:
status: New → Triaged
importance: Undecided → High
Revision history for this message
Dimitri John Ledkov (xnox) wrote :

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

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

See bzrlib/config.py:514

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

Changed in bzr:
status: New → Confirmed
importance: Undecided → High
Changed in bzr-svn:
importance: Medium → High
Revision history for this message
Dimitri John Ledkov (xnox) wrote :

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.....

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

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...

Revision history for this message
Tres Seaver (tseaver) wrote :
Download full text (4.0 KiB)

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...

Read more...

Tim Penhey (thumper)
tags: added: code-import
Revision history for this message
Dimitri John Ledkov (xnox) wrote : Re: [Bug 525571] Re: bzr svn-import fails when invoked concurrently

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 =)

Revision history for this message
Robert Collins (lifeless) wrote : Re: bzr svn-import fails when invoked concurrently

@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.

Tom Haddon (mthaddon)
tags: added: canonical-losa-lp
Revision history for this message
Tom Haddon (mthaddon) wrote :

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

Jelmer Vernooij (jelmer)
summary: - bzr svn-import fails when invoked concurrently
+ bzrlib.config.IniBasedConfig doesn't lock
Revision history for this message
Robert Collins (lifeless) wrote : Re: [Bug 525571] Re: bzrlib.config.IniBasedConfig doesn't lock

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.

Revision history for this message
Jelmer Vernooij (jelmer) wrote : Re: LocationsConfig doesn't lock around bzrlib.config.IniBasedConfig

Fair enough - updated.

summary: - bzrlib.config.IniBasedConfig doesn't lock
+ LocationsConfig doesn't lock around bzrlib.config.IniBasedConfig
Jelmer Vernooij (jelmer)
summary: - LocationsConfig doesn't lock around bzrlib.config.IniBasedConfig
+ No locking when updating files in ~/.bazaar
Revision history for this message
Marcos Garcia (marcosgdf) wrote : Re: [Bug 525571] Re: No locking when updating files in ~/.bazaar

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>

Vincent Ladeuil (vila)
tags: added: udd
Martin Pool (mbp)
Changed in bzr:
milestone: none → 2.2.0
Vincent Ladeuil (vila)
Changed in bzr:
assignee: nobody → Vincent Ladeuil (vila)
status: Confirmed → In Progress
Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Revision history for this message
Jelmer Vernooij (jelmer) wrote :
Revision history for this message
Vincent Ladeuil (vila) wrote :

Thanks jelmer, now does someone have a corrupted one ?

Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Tim Penhey (thumper) wrote :

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.

Revision history for this message
Robert Collins (lifeless) wrote :

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

-Rob

Revision history for this message
Vincent Ladeuil (vila) wrote :

>>>>> "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.

Revision history for this message
Steve McInerney (spm) wrote :

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

Revision history for this message
Steve McInerney (spm) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :

@Steve: thanks.

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

Revision history for this message
Vincent Ladeuil (vila) wrote :

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.

Changed in bzr:
milestone: 2.2.0 → 2.1.3
status: In Progress → Fix Released
Vincent Ladeuil (vila)
Changed in bzr-svn:
status: Triaged → In Progress
assignee: nobody → Vincent Ladeuil (vila)
Jelmer Vernooij (jelmer)
Changed in bzr-svn:
status: In Progress → Fix Committed
milestone: none → 1.0.3
status: Fix Committed → In Progress
Jelmer Vernooij (jelmer)
Changed in bzr-svn:
status: In Progress → Fix Committed
Jelmer Vernooij (jelmer)
Changed in bzr-svn:
status: Fix Committed → Fix Released
Revision history for this message
Launchpad QA Bot (lpqabot) wrote : Bug fixed by a commit
Changed in launchpad-code:
assignee: nobody → Jelmer Vernooij (jelmer)
milestone: none → 10.08
tags: added: qa-needstesting
Changed in launchpad-code:
status: Triaged → Fix Committed
Revision history for this message
Ursula Junque (ursinha) wrote :
Revision history for this message
Ursula Junque (ursinha) wrote :

This was tested by mwhudson on staging.

tags: added: qa-ok
removed: qa-needstesting
Changed in launchpad-code:
status: Fix Committed → Fix Released
Revision history for this message
Vincent Ladeuil (vila) wrote :

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.

Revision history for this message
Vincent Ladeuil (vila) wrote :

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.

Vincent Ladeuil (vila)
tags: added: sru
Vincent Ladeuil (vila)
Changed in bzr (Ubuntu):
status: New → Fix Released
Revision history for this message
Martin Pitt (pitti) wrote :

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

Changed in bzr (Ubuntu Lucid):
status: New → Triaged
Changed in bzr (Ubuntu Maverick):
status: Fix Released → Triaged
Vincent Ladeuil (vila)
Changed in bzr (Ubuntu Maverick):
status: Triaged → Confirmed
Revision history for this message
Martin Pitt (pitti) wrote : Please test proposed package

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!

Changed in bzr (Ubuntu Maverick):
status: Confirmed → Fix Committed
tags: added: verification-needed
Revision history for this message
Launchpad Janitor (janitor) wrote :

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

Changed in bzr (Ubuntu Maverick):
status: Fix Committed → Fix Released
Revision history for this message
Martin Pitt (pitti) wrote :

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

Changed in bzr (Ubuntu):
status: Confirmed → Fix Released
Vincent Ladeuil (vila)
tags: added: config
Revision history for this message
Clint Byrum (clint-fewbar) wrote :

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!

Changed in bzr (Ubuntu Lucid):
status: Triaged → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

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

Changed in bzr (Ubuntu Lucid):
status: Fix Committed → Fix Released
Martin Packman (gz)
tags: added: affects-twisted
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Related questions