Merge lp:~spiv/bzr/lockcontention-pushing-tag-733350-2.3 into lp:bzr/2.3

Proposed by Andrew Bennetts
Status: Merged
Approved by: Andrew Bennetts
Approved revision: no longer in the source branch.
Merged at revision: 5629
Proposed branch: lp:~spiv/bzr/lockcontention-pushing-tag-733350-2.3
Merge into: lp:bzr/2.3
Diff against target: 184 lines (+99/-7)
4 files modified
bzrlib/branch.py (+11/-7)
bzrlib/tests/per_branch/test_branch.py (+67/-0)
bzrlib/tests/per_branch/test_push.py (+17/-0)
doc/en/release-notes/bzr-2.3.txt (+4/-0)
To merge this branch: bzr merge lp:~spiv/bzr/lockcontention-pushing-tag-733350-2.3
Reviewer Review Type Date Requested Status
Martin Pool Approve
bzr-core Pending
Review via email: mp+53947@code.launchpad.net

Commit message

Fix LockContention error when pushing new tags to bound branch (#733350)

Description of the change

Fixes a regression in 2.3.0: LockContention errors when pushing a new tag to a bound branch, and adds a test that fails without the fix.

The problem is that GenericInterBranch._push_with_bound_branches was getting and locking the master branch, then calling Branch._basic_push which in turn calls b.tags.merge_to, which now by default also gets and locks the master branch.

This patch fixes that by adding caching of the return value of get_master_branch (as suggested by a code comment from 2006!), so that both places now lock_write the same branch. This possibly adds some small performance improvement for other cases? I've added tests that the cache is invalidated when it should be (without requiring that branch implementations necessarily cache at all).

The other way to fix this would be by passing an extra parameter to _basic_push to tell it to pass ignore_master=True to tags.merge_to, but this would break plugins like bzr-svn that implement _basic_push (despite it's apparently private name). If you look back in the revision history of this branch you'll see I implemented a hackish solution for this by essentially passing the param via setting and unsetting a private instance variable that our _basic_push implementation would check, but this seemed just too ugly, even if in some ways it's a more conservative fix (and this is targeted to a stable branch).

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

PEP8: docstrings ought to have just one sentence on the first line. (I guess you accidentally reflowed them.)

Thanks for fixing this and for the clear cover letter. It makes sense to me.

I'd like a docstring mention for the new ivar.

You may want to wait for a second review in case someone knows of problem consequences.

(tweak)

review: Approve
Revision history for this message
Andrew Bennetts (spiv) wrote :

Martin Pool wrote:
> Review: Approve
> PEP8: docstrings ought to have just one sentence on the first line.
> (I guess you accidentally reflowed them.)

Changed (although the pydoctor output treats a first paragraph
identically to a first line, so the practical benefit seems low).

> I'd like a docstring mention for the new ivar.

Done (although pydoctor doesn't appear to notice that underscored ivar
names are private, probably because epydoc doesn't have a public/private
distinction here either? Anyway documenting things is good, and our
conventions are pretty clear and well-recognised even though we have
lots of exceptions like _format).

> You may want to wait for a second review in case someone knows of
> problem consequences.

Good idea. I'll wait. I hope there aren't any!

Thanks for the review!

Revision history for this message
John A Meinel (jameinel) wrote :

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

On 3/18/2011 7:46 AM, Andrew Bennetts wrote:
> Martin Pool wrote:
>> Review: Approve
>> PEP8: docstrings ought to have just one sentence on the first line.
>> (I guess you accidentally reflowed them.)
>
> Changed (although the pydoctor output treats a first paragraph
> identically to a first line, so the practical benefit seems low).
>
>> I'd like a docstring mention for the new ivar.
>
> Done (although pydoctor doesn't appear to notice that underscored ivar
> names are private, probably because epydoc doesn't have a public/private
> distinction here either? Anyway documenting things is good, and our
> conventions are pretty clear and well-recognised even though we have
> lots of exceptions like _format).
>
>> You may want to wait for a second review in case someone knows of
>> problem consequences.
>
> Good idea. I'll wait. I hope there aren't any!
>
> Thanks for the review!
>

Check if I understand correctly.

The issue is that we pulled tags from the master, which updates are
local copy, which we then try to push back up to the master. We have
code in 'pull' that already sees 'if source == master:
dont_update_master' for the fetch case.

However, at the time we get to merging tags, we have forgotten that the
source was the master, and thus we shouldn't be trying to update it.

What I don't like about your fix, is that we will upload all the tags
back to the master, even though we just downloaded them *from* the master.

I do like caching the master branch.

As for changing _basic_push, I think we can just do it. If you really
want compatibility, you could do:

try:
  self._basic_push(..., new_flag=XXX)
except (TypeError, AttributeError):
  self._basic_push(...)

As for public/private, it has always been a little unclear what is
private (not meant to be directly called by people using Branch) vs
Private (not meant to be implemented by a subclass).

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

iEYEARECAAYFAk2DKZ4ACgkQJdeBCYSNAAMVyQCgsConJMIA7xCnPOjw/cwcmIM1
E/wAoJ/YCX9towwhx5yyYHUnfR3+21nv
=SU/r
-----END PGP SIGNATURE-----

Revision history for this message
Andrew Bennetts (spiv) wrote :

John Arbash Meinel wrote:
[…]
> What I don't like about your fix, is that we will upload all the tags
> back to the master, even though we just downloaded them *from* the master.

We discussed on IRC, repeating here for posterity: we already cache the
tags, so actually no extra reads or writes of the master's tags will
occur in this case. I forget to state this in my cover letter. (The
exception is if for some reason the checkout is up to date w.r.t. the
master, but somehow has different tags. That's theoretically possible
but would be very unusual.)

> I do like caching the master branch.
>
> As for changing _basic_push, I think we can just do it. If you really
> want compatibility, you could do:

Not in a stable branch: this is fixing a regression in 2.3, and I don't
think we want 2.3.2 to suddenly break bzr-svn (which isn't even affected
by this bug)!

It may be nice to change in trunk, but given that this fix works well
already I'm not sure it's worth the effort.

-Andrew.

Revision history for this message
Martin Packman (gz) wrote :

I ran the tests locally and they all pass.

Revision history for this message
Andrew Bennetts (spiv) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py 2011-01-12 01:01:53 +0000
+++ bzrlib/branch.py 2011-03-18 04:58:17 +0000
@@ -94,6 +94,7 @@
94 self._partial_revision_history_cache = []94 self._partial_revision_history_cache = []
95 self._tags_bytes = None95 self._tags_bytes = None
96 self._last_revision_info_cache = None96 self._last_revision_info_cache = None
97 self._master_branch_cache = None
97 self._merge_sorted_revisions_cache = None98 self._merge_sorted_revisions_cache = None
98 self._open_hook()99 self._open_hook()
99 hooks = Branch.hooks['open']100 hooks = Branch.hooks['open']
@@ -926,6 +927,7 @@
926 self._revision_history_cache = None927 self._revision_history_cache = None
927 self._revision_id_to_revno_cache = None928 self._revision_id_to_revno_cache = None
928 self._last_revision_info_cache = None929 self._last_revision_info_cache = None
930 self._master_branch_cache = None
929 self._merge_sorted_revisions_cache = None931 self._merge_sorted_revisions_cache = None
930 self._partial_revision_history_cache = []932 self._partial_revision_history_cache = []
931 self._partial_revision_id_to_revno_cache = {}933 self._partial_revision_id_to_revno_cache = {}
@@ -2671,8 +2673,7 @@
2671 target.update_revisions(self, stop_revision,2673 target.update_revisions(self, stop_revision,
2672 overwrite=overwrite, graph=graph)2674 overwrite=overwrite, graph=graph)
2673 if self._push_should_merge_tags():2675 if self._push_should_merge_tags():
2674 result.tag_conflicts = self.tags.merge_to(target.tags,2676 result.tag_conflicts = self.tags.merge_to(target.tags, overwrite)
2675 overwrite)
2676 result.new_revno, result.new_revid = target.last_revision_info()2677 result.new_revno, result.new_revid = target.last_revision_info()
2677 return result2678 return result
26782679
@@ -2723,12 +2724,13 @@
2723 """Return the branch we are bound to.2724 """Return the branch we are bound to.
27242725
2725 :return: Either a Branch, or None2726 :return: Either a Branch, or None
2726
2727 This could memoise the branch, but if thats done
2728 it must be revalidated on each new lock.
2729 So for now we just don't memoise it.
2730 # RBC 20060304 review this decision.
2731 """2727 """
2728 if self._master_branch_cache is None:
2729 self._master_branch_cache = self._get_master_branch(
2730 possible_transports)
2731 return self._master_branch_cache
2732
2733 def _get_master_branch(self, possible_transports):
2732 bound_loc = self.get_bound_location()2734 bound_loc = self.get_bound_location()
2733 if not bound_loc:2735 if not bound_loc:
2734 return None2736 return None
@@ -2745,6 +2747,7 @@
27452747
2746 :param location: URL to the target branch2748 :param location: URL to the target branch
2747 """2749 """
2750 self._master_branch_cache = None
2748 if location:2751 if location:
2749 self._transport.put_bytes('bound', location+'\n',2752 self._transport.put_bytes('bound', location+'\n',
2750 mode=self.bzrdir._get_file_mode())2753 mode=self.bzrdir._get_file_mode())
@@ -3002,6 +3005,7 @@
30023005
3003 def set_bound_location(self, location):3006 def set_bound_location(self, location):
3004 """See Branch.set_push_location."""3007 """See Branch.set_push_location."""
3008 self._master_branch_cache = None
3005 result = None3009 result = None
3006 config = self.get_config()3010 config = self.get_config()
3007 if location is None:3011 if location is None:
30083012
=== modified file 'bzrlib/tests/per_branch/test_branch.py'
--- bzrlib/tests/per_branch/test_branch.py 2011-01-27 14:27:18 +0000
+++ bzrlib/tests/per_branch/test_branch.py 2011-03-18 04:58:17 +0000
@@ -746,6 +746,73 @@
746 except errors.UpgradeRequired:746 except errors.UpgradeRequired:
747 raise tests.TestNotApplicable('Format does not support binding')747 raise tests.TestNotApplicable('Format does not support binding')
748748
749 def test_unbind_clears_cached_master_branch(self):
750 """b.get_master_branch may return a cached branch if b is locked. That
751 cache should be cleared if the master branch is changed via unbind.
752 """
753 master = self.make_branch('master')
754 branch = self.make_branch('branch')
755 try:
756 branch.bind(master)
757 except errors.UpgradeRequired:
758 raise tests.TestNotApplicable('Format does not support binding')
759 self.addCleanup(branch.lock_write().unlock)
760 self.assertNotEqual(None, branch.get_master_branch())
761 branch.unbind()
762 self.assertEqual(None, branch.get_master_branch())
763
764 def test_unlocked_does_not_cache_master_branch(self):
765 """Unlocked branches do not cache the result of get_master_branch."""
766 master = self.make_branch('master')
767 branch1 = self.make_branch('branch')
768 try:
769 branch1.bind(master)
770 except errors.UpgradeRequired:
771 raise tests.TestNotApplicable('Format does not support binding')
772 # Open branch1 again
773 branch2 = branch1.bzrdir.open_branch()
774 self.assertNotEqual(None, branch1.get_master_branch())
775 # Unbind the branch via branch2. branch1 isn't locked so will
776 # immediately return the new value for get_master_branch.
777 branch2.unbind()
778 self.assertEqual(None, branch1.get_master_branch())
779
780 def test_bind_clears_cached_master_branch(self):
781 """b.get_master_branch may return a cached branch if b is locked. That
782 cache should be cleared if the master branch is changed via
783 set_bound_location.
784 """
785 master1 = self.make_branch('master1')
786 master2 = self.make_branch('master2')
787 branch = self.make_branch('branch')
788 try:
789 branch.bind(master1)
790 except errors.UpgradeRequired:
791 raise tests.TestNotApplicable('Format does not support binding')
792 self.addCleanup(branch.lock_write().unlock)
793 self.assertNotEqual(None, branch.get_master_branch())
794 branch.bind(master2)
795 self.assertEqual('.', urlutils.relative_url(self.get_url('master2'),
796 branch.get_master_branch().base))
797
798 def test_set_bound_location_clears_cached_master_branch(self):
799 """b.get_master_branch may return a cached branch if b is locked. That
800 cache should be cleared if the master branch is changed via
801 set_bound_location.
802 """
803 master1 = self.make_branch('master1')
804 master2 = self.make_branch('master2')
805 branch = self.make_branch('branch')
806 try:
807 branch.bind(master1)
808 except errors.UpgradeRequired:
809 raise tests.TestNotApplicable('Format does not support binding')
810 self.addCleanup(branch.lock_write().unlock)
811 self.assertNotEqual(None, branch.get_master_branch())
812 branch.set_bound_location(self.get_url('master2'))
813 self.assertEqual('.', urlutils.relative_url(self.get_url('master2'),
814 branch.get_master_branch().base))
815
749816
750class TestStrict(per_branch.TestCaseWithBranch):817class TestStrict(per_branch.TestCaseWithBranch):
751818
752819
=== modified file 'bzrlib/tests/per_branch/test_push.py'
--- bzrlib/tests/per_branch/test_push.py 2011-01-13 01:02:53 +0000
+++ bzrlib/tests/per_branch/test_push.py 2011-03-18 04:58:17 +0000
@@ -114,6 +114,23 @@
114 self.assertRaises(errors.BoundBranchConnectionFailure,114 self.assertRaises(errors.BoundBranchConnectionFailure,
115 other.branch.push, checkout.branch)115 other.branch.push, checkout.branch)
116116
117 def test_push_new_tag_to_bound_branch(self):
118 master = self.make_branch('master')
119 bound = self.make_branch('bound')
120 try:
121 bound.bind(master)
122 except errors.UpgradeRequired:
123 raise tests.TestNotApplicable(
124 'Format does not support bound branches')
125 other = bound.bzrdir.sprout('other').open_branch()
126 try:
127 other.tags.set_tag('new-tag', 'some-rev')
128 except errors.TagsNotSupported:
129 raise tests.TestNotApplicable('Format does not support tags')
130 other.push(bound)
131 self.assertEqual({'new-tag': 'some-rev'}, bound.tags.get_tag_dict())
132 self.assertEqual({'new-tag': 'some-rev'}, master.tags.get_tag_dict())
133
117 def test_push_uses_read_lock(self):134 def test_push_uses_read_lock(self):
118 """Push should only need a read lock on the source side."""135 """Push should only need a read lock on the source side."""
119 source = self.make_branch_and_tree('source')136 source = self.make_branch_and_tree('source')
120137
=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt 2011-03-14 10:19:22 +0000
+++ doc/en/release-notes/bzr-2.3.txt 2011-03-18 04:58:17 +0000
@@ -32,6 +32,10 @@
32.. Fixes for situations where bzr would previously crash or give incorrect32.. Fixes for situations where bzr would previously crash or give incorrect
33 or undesirable results.33 or undesirable results.
3434
35* Fix "Unable to obtain lock" error when pushing to a bound branch if tags
36 had changed. Bazaar was attempting to open and lock the master branch
37 twice in this case. (Andrew Bennetts, #733350)
38
35Documentation39Documentation
36*************40*************
3741

Subscribers

People subscribed via source and target branches