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

[Andrew Bennetts]

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

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

[Martin Pool (mbp)]

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

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

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

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

I ran the tests locally and they all pass.

[Andrew Bennetts (spiv)]

sent to pqm by email

=== 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 @@
         self._partial_revision_history_cache = []
         self._tags_bytes = None
         self._last_revision_info_cache = None
+        self._master_branch_cache = None
         self._merge_sorted_revisions_cache = None
         self._open_hook()
         hooks = Branch.hooks['open']
@@ -926,6 +927,7 @@
         self._revision_history_cache = None
         self._revision_id_to_revno_cache = None
         self._last_revision_info_cache = None
+        self._master_branch_cache = None
         self._merge_sorted_revisions_cache = None
         self._partial_revision_history_cache = []
         self._partial_revision_id_to_revno_cache = {}
@@ -2671,8 +2673,7 @@
             target.update_revisions(self, stop_revision,
                 overwrite=overwrite, graph=graph)
         if self._push_should_merge_tags():
-            result.tag_conflicts = self.tags.merge_to(target.tags,
-                overwrite)
+            result.tag_conflicts = self.tags.merge_to(target.tags, overwrite)
         result.new_revno, result.new_revid = target.last_revision_info()
         return result
 
@@ -2723,12 +2724,13 @@
         """Return the branch we are bound to.
 
         :return: Either a Branch, or None
-
-        This could memoise the branch, but if thats done
-        it must be revalidated on each new lock.
-        So for now we just don't memoise it.
-        # RBC 20060304 review this decision.
         """
+        if self._master_branch_cache is None:
+            self._master_branch_cache = self._get_master_branch(
+                possible_transports)
+        return self._master_branch_cache
+
+    def _get_master_branch(self, possible_transports):
         bound_loc = self.get_bound_location()
         if not bound_loc:
             return None
@@ -2745,6 +2747,7 @@
 
         :param location: URL to the target branch
         """
+        self._master_branch_cache = None
         if location:
             self._transport.put_bytes('bound', location+'\n',
                 mode=self.bzrdir._get_file_mode())
@@ -3002,6 +3005,7 @@
 
     def set_bound_location(self, location):
         """See Branch.set_push_location."""
+        self._master_branch_cache = None
         result = None
         config = self.get_config()
         if location is None:

=== 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 @@
         except errors.UpgradeRequired:
             raise tests.TestNotApplicable('Format does not support binding')
 
+    def test_unbind_clears_cached_master_branch(self):
+        """b.get_master_branch may return a cached branch if b is locked.  That
+        cache should be cleared if the master branch is changed via unbind.
+        """
+        master = self.make_branch('master')
+        branch = self.make_branch('branch')
+        try:
+            branch.bind(master)
+        except errors.UpgradeRequired:
+            raise tests.TestNotApplicable('Format does not support binding')
+        self.addCleanup(branch.lock_write().unlock)
+        self.assertNotEqual(None, branch.get_master_branch())
+        branch.unbind()
+        self.assertEqual(None, branch.get_master_branch())
+
+    def test_unlocked_does_not_cache_master_branch(self):
+        """Unlocked branches do not cache the result of get_master_branch."""
+        master = self.make_branch('master')
+        branch1 = self.make_branch('branch')
+        try:
+            branch1.bind(master)
+        except errors.UpgradeRequired:
+            raise tests.TestNotApplicable('Format does not support binding')
+        # Open branch1 again
+        branch2 = branch1.bzrdir.open_branch()
+        self.assertNotEqual(None, branch1.get_master_branch())
+        # Unbind the branch via branch2.  branch1 isn't locked so will
+        # immediately return the new value for get_master_branch.
+        branch2.unbind()
+        self.assertEqual(None, branch1.get_master_branch())
+
+    def test_bind_clears_cached_master_branch(self):
+        """b.get_master_branch may return a cached branch if b is locked.  That
+        cache should be cleared if the master branch is changed via
+        set_bound_location.
+        """
+        master1 = self.make_branch('master1')
+        master2 = self.make_branch('master2')
+        branch = self.make_branch('branch')
+        try:
+            branch.bind(master1)
+        except errors.UpgradeRequired:
+            raise tests.TestNotApplicable('Format does not support binding')
+        self.addCleanup(branch.lock_write().unlock)
+        self.assertNotEqual(None, branch.get_master_branch())
+        branch.bind(master2)
+        self.assertEqual('.', urlutils.relative_url(self.get_url('master2'),
+                branch.get_master_branch().base))
+
+    def test_set_bound_location_clears_cached_master_branch(self):
+        """b.get_master_branch may return a cached branch if b is locked.  That
+        cache should be cleared if the master branch is changed via
+        set_bound_location.
+        """
+        master1 = self.make_branch('master1')
+        master2 = self.make_branch('master2')
+        branch = self.make_branch('branch')
+        try:
+            branch.bind(master1)
+        except errors.UpgradeRequired:
+            raise tests.TestNotApplicable('Format does not support binding')
+        self.addCleanup(branch.lock_write().unlock)
+        self.assertNotEqual(None, branch.get_master_branch())
+        branch.set_bound_location(self.get_url('master2'))
+        self.assertEqual('.', urlutils.relative_url(self.get_url('master2'),
+                branch.get_master_branch().base))
+
 
 class TestStrict(per_branch.TestCaseWithBranch):
 

=== 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 @@
         self.assertRaises(errors.BoundBranchConnectionFailure,
                 other.branch.push, checkout.branch)
 
+    def test_push_new_tag_to_bound_branch(self):
+        master = self.make_branch('master')
+        bound = self.make_branch('bound')
+        try:
+            bound.bind(master)
+        except errors.UpgradeRequired:
+            raise tests.TestNotApplicable(
+                'Format does not support bound branches')
+        other = bound.bzrdir.sprout('other').open_branch()
+        try:
+            other.tags.set_tag('new-tag', 'some-rev')
+        except errors.TagsNotSupported:
+            raise tests.TestNotApplicable('Format does not support tags')
+        other.push(bound)
+        self.assertEqual({'new-tag': 'some-rev'}, bound.tags.get_tag_dict())
+        self.assertEqual({'new-tag': 'some-rev'}, master.tags.get_tag_dict())
+
     def test_push_uses_read_lock(self):
         """Push should only need a read lock on the source side."""
         source = self.make_branch_and_tree('source')

=== 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 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* Fix "Unable to obtain lock" error when pushing to a bound branch if tags
+  had changed.  Bazaar was attempting to open and lock the master branch
+  twice in this case.  (Andrew Bennetts, #733350)
+
 Documentation
 *************