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

Proposed by Andrew Bennetts on 2011-03-18
Status: Merged
Approved by: Andrew Bennetts on 2011-03-21
Approved revision: 5633
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 2011-03-18 Approve on 2011-03-18
bzr-core 2011-03-18 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.
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
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!

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

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.

Martin Packman (gz) wrote :

I ran the tests locally and they all pass.

Andrew Bennetts (spiv) wrote :

sent to pqm by email

5634. By Andrew Bennetts on 2011-03-18

Docstring tweaks.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/branch.py'
2--- bzrlib/branch.py 2011-01-12 01:01:53 +0000
3+++ bzrlib/branch.py 2011-03-18 04:58:17 +0000
4@@ -94,6 +94,7 @@
5 self._partial_revision_history_cache = []
6 self._tags_bytes = None
7 self._last_revision_info_cache = None
8+ self._master_branch_cache = None
9 self._merge_sorted_revisions_cache = None
10 self._open_hook()
11 hooks = Branch.hooks['open']
12@@ -926,6 +927,7 @@
13 self._revision_history_cache = None
14 self._revision_id_to_revno_cache = None
15 self._last_revision_info_cache = None
16+ self._master_branch_cache = None
17 self._merge_sorted_revisions_cache = None
18 self._partial_revision_history_cache = []
19 self._partial_revision_id_to_revno_cache = {}
20@@ -2671,8 +2673,7 @@
21 target.update_revisions(self, stop_revision,
22 overwrite=overwrite, graph=graph)
23 if self._push_should_merge_tags():
24- result.tag_conflicts = self.tags.merge_to(target.tags,
25- overwrite)
26+ result.tag_conflicts = self.tags.merge_to(target.tags, overwrite)
27 result.new_revno, result.new_revid = target.last_revision_info()
28 return result
29
30@@ -2723,12 +2724,13 @@
31 """Return the branch we are bound to.
32
33 :return: Either a Branch, or None
34-
35- This could memoise the branch, but if thats done
36- it must be revalidated on each new lock.
37- So for now we just don't memoise it.
38- # RBC 20060304 review this decision.
39 """
40+ if self._master_branch_cache is None:
41+ self._master_branch_cache = self._get_master_branch(
42+ possible_transports)
43+ return self._master_branch_cache
44+
45+ def _get_master_branch(self, possible_transports):
46 bound_loc = self.get_bound_location()
47 if not bound_loc:
48 return None
49@@ -2745,6 +2747,7 @@
50
51 :param location: URL to the target branch
52 """
53+ self._master_branch_cache = None
54 if location:
55 self._transport.put_bytes('bound', location+'\n',
56 mode=self.bzrdir._get_file_mode())
57@@ -3002,6 +3005,7 @@
58
59 def set_bound_location(self, location):
60 """See Branch.set_push_location."""
61+ self._master_branch_cache = None
62 result = None
63 config = self.get_config()
64 if location is None:
65
66=== modified file 'bzrlib/tests/per_branch/test_branch.py'
67--- bzrlib/tests/per_branch/test_branch.py 2011-01-27 14:27:18 +0000
68+++ bzrlib/tests/per_branch/test_branch.py 2011-03-18 04:58:17 +0000
69@@ -746,6 +746,73 @@
70 except errors.UpgradeRequired:
71 raise tests.TestNotApplicable('Format does not support binding')
72
73+ def test_unbind_clears_cached_master_branch(self):
74+ """b.get_master_branch may return a cached branch if b is locked. That
75+ cache should be cleared if the master branch is changed via unbind.
76+ """
77+ master = self.make_branch('master')
78+ branch = self.make_branch('branch')
79+ try:
80+ branch.bind(master)
81+ except errors.UpgradeRequired:
82+ raise tests.TestNotApplicable('Format does not support binding')
83+ self.addCleanup(branch.lock_write().unlock)
84+ self.assertNotEqual(None, branch.get_master_branch())
85+ branch.unbind()
86+ self.assertEqual(None, branch.get_master_branch())
87+
88+ def test_unlocked_does_not_cache_master_branch(self):
89+ """Unlocked branches do not cache the result of get_master_branch."""
90+ master = self.make_branch('master')
91+ branch1 = self.make_branch('branch')
92+ try:
93+ branch1.bind(master)
94+ except errors.UpgradeRequired:
95+ raise tests.TestNotApplicable('Format does not support binding')
96+ # Open branch1 again
97+ branch2 = branch1.bzrdir.open_branch()
98+ self.assertNotEqual(None, branch1.get_master_branch())
99+ # Unbind the branch via branch2. branch1 isn't locked so will
100+ # immediately return the new value for get_master_branch.
101+ branch2.unbind()
102+ self.assertEqual(None, branch1.get_master_branch())
103+
104+ def test_bind_clears_cached_master_branch(self):
105+ """b.get_master_branch may return a cached branch if b is locked. That
106+ cache should be cleared if the master branch is changed via
107+ set_bound_location.
108+ """
109+ master1 = self.make_branch('master1')
110+ master2 = self.make_branch('master2')
111+ branch = self.make_branch('branch')
112+ try:
113+ branch.bind(master1)
114+ except errors.UpgradeRequired:
115+ raise tests.TestNotApplicable('Format does not support binding')
116+ self.addCleanup(branch.lock_write().unlock)
117+ self.assertNotEqual(None, branch.get_master_branch())
118+ branch.bind(master2)
119+ self.assertEqual('.', urlutils.relative_url(self.get_url('master2'),
120+ branch.get_master_branch().base))
121+
122+ def test_set_bound_location_clears_cached_master_branch(self):
123+ """b.get_master_branch may return a cached branch if b is locked. That
124+ cache should be cleared if the master branch is changed via
125+ set_bound_location.
126+ """
127+ master1 = self.make_branch('master1')
128+ master2 = self.make_branch('master2')
129+ branch = self.make_branch('branch')
130+ try:
131+ branch.bind(master1)
132+ except errors.UpgradeRequired:
133+ raise tests.TestNotApplicable('Format does not support binding')
134+ self.addCleanup(branch.lock_write().unlock)
135+ self.assertNotEqual(None, branch.get_master_branch())
136+ branch.set_bound_location(self.get_url('master2'))
137+ self.assertEqual('.', urlutils.relative_url(self.get_url('master2'),
138+ branch.get_master_branch().base))
139+
140
141 class TestStrict(per_branch.TestCaseWithBranch):
142
143
144=== modified file 'bzrlib/tests/per_branch/test_push.py'
145--- bzrlib/tests/per_branch/test_push.py 2011-01-13 01:02:53 +0000
146+++ bzrlib/tests/per_branch/test_push.py 2011-03-18 04:58:17 +0000
147@@ -114,6 +114,23 @@
148 self.assertRaises(errors.BoundBranchConnectionFailure,
149 other.branch.push, checkout.branch)
150
151+ def test_push_new_tag_to_bound_branch(self):
152+ master = self.make_branch('master')
153+ bound = self.make_branch('bound')
154+ try:
155+ bound.bind(master)
156+ except errors.UpgradeRequired:
157+ raise tests.TestNotApplicable(
158+ 'Format does not support bound branches')
159+ other = bound.bzrdir.sprout('other').open_branch()
160+ try:
161+ other.tags.set_tag('new-tag', 'some-rev')
162+ except errors.TagsNotSupported:
163+ raise tests.TestNotApplicable('Format does not support tags')
164+ other.push(bound)
165+ self.assertEqual({'new-tag': 'some-rev'}, bound.tags.get_tag_dict())
166+ self.assertEqual({'new-tag': 'some-rev'}, master.tags.get_tag_dict())
167+
168 def test_push_uses_read_lock(self):
169 """Push should only need a read lock on the source side."""
170 source = self.make_branch_and_tree('source')
171
172=== modified file 'doc/en/release-notes/bzr-2.3.txt'
173--- doc/en/release-notes/bzr-2.3.txt 2011-03-14 10:19:22 +0000
174+++ doc/en/release-notes/bzr-2.3.txt 2011-03-18 04:58:17 +0000
175@@ -32,6 +32,10 @@
176 .. Fixes for situations where bzr would previously crash or give incorrect
177 or undesirable results.
178
179+* Fix "Unable to obtain lock" error when pushing to a bound branch if tags
180+ had changed. Bazaar was attempting to open and lock the master branch
181+ twice in this case. (Andrew Bennetts, #733350)
182+
183 Documentation
184 *************
185

Subscribers

People subscribed via source and target branches