Merge lp:~spiv/bzr/hpss-diff-locking-bug into lp:bzr/2.0

Proposed by Andrew Bennetts on 2009-10-07
Status: Merged
Merged at revision: not available
Proposed branch: lp:~spiv/bzr/hpss-diff-locking-bug
Merge into: lp:bzr/2.0
Diff against target: 55 lines
3 files modified
NEWS (+3/-0)
bzrlib/remote.py (+1/-0)
bzrlib/tests/test_remote.py (+20/-0)
To merge this branch: bzr merge lp:~spiv/bzr/hpss-diff-locking-bug
Reviewer Review Type Date Requested Status
Vincent Ladeuil 2009-10-07 Approve on 2009-10-07
Review via email: mp+12965@code.launchpad.net
To post a comment you must log in.
Andrew Bennetts (spiv) wrote :

This fixes bug 389413. Various code paths assume that Branch.get_rev_id will implicitly lock_read the branch (and repository), but RemoteBranch's get_rev_id didn't have the @needs_read_lock decorator. So this branch fixes that, and adds a test about it. The sneaky part is that RemoteBranch would get away with it if the server is new enough, which is probably why the existing test suite didn't catch this.

Also, it's not so great that callers such as revisionspec.py expect get_rev_id to acquire a read lock for them, as we are almost certainly wasting some effort, but fixing the performance is separate to fixing the correctness. (And fixing the performance is considerable knottier.)

I think this fix is a good candidate for the 2.0 branch.

Vincent Ladeuil (vila) wrote :

Clear, concise, nice job :)
I agree that should be backported to 2.0.x.

review: Approve
John A Meinel (jameinel) wrote :

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

Vincent Ladeuil wrote:
> Review: Approve
> Clear, concise, nice job :)
> I agree that should be backported to 2.0.x.

Overall, I would like to see us get away from @needs_read_lock
decorators, because they inherently hide performance issues.

That said, good enough for a bugfix, I guess.

John
=:->

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

iEYEARECAAYFAkrMmIQACgkQJdeBCYSNAANJbgCgwHUcc3Xwb/ReidSTi3fF8xPL
J7EAnjNf4uixV+lKLkqKtk4KXvtzrGWI
=ARY2
-----END PGP SIGNATURE-----

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-10-07 00:46:40 +0000
3+++ NEWS 2009-10-07 05:45:22 +0000
4@@ -26,6 +26,9 @@
5 with some combinations of remote and local formats. This was causing
6 "unknown object type identifier 60" errors. (Andrew Bennetts, #427736)
7
8+* Fixed ``ObjectNotLocked`` errors when doing some log and diff operations
9+ on branches via a smart server. (Andrew Bennetts, #389413)
10+
11 * Occasional IndexError on renamed files have been fixed. Operations that
12 set a full inventory in the working tree will now go via the
13 apply_inventory_delta code path which is simpler and easier to
14
15=== modified file 'bzrlib/remote.py'
16--- bzrlib/remote.py 2009-10-02 03:07:22 +0000
17+++ bzrlib/remote.py 2009-10-07 05:45:22 +0000
18@@ -2392,6 +2392,7 @@
19 raise NotImplementedError(self.dont_leave_lock_in_place)
20 self._leave_lock = False
21
22+ @needs_read_lock
23 def get_rev_id(self, revno, history=None):
24 if revno == 0:
25 return _mod_revision.NULL_REVISION
26
27=== modified file 'bzrlib/tests/test_remote.py'
28--- bzrlib/tests/test_remote.py 2009-09-10 06:32:42 +0000
29+++ bzrlib/tests/test_remote.py 2009-10-07 05:45:22 +0000
30@@ -2147,6 +2147,26 @@
31 repo.get_rev_id_for_revno, 5, (42, 'rev-foo'))
32 self.assertFinished(client)
33
34+ def test_branch_fallback_locking(self):
35+ """RemoteBranch.get_rev_id takes a read lock, and tries to call the
36+ get_rev_id_for_revno verb. If the verb is unknown the VFS fallback
37+ will be invoked, which will fail if the repo is unlocked.
38+ """
39+ self.setup_smart_server_with_call_log()
40+ tree = self.make_branch_and_memory_tree('.')
41+ tree.lock_write()
42+ rev1 = tree.commit('First')
43+ rev2 = tree.commit('Second')
44+ tree.unlock()
45+ branch = tree.branch
46+ self.assertFalse(branch.is_locked())
47+ self.reset_smart_call_log()
48+ verb = 'Repository.get_rev_id_for_revno'
49+ self.disable_verb(verb)
50+ self.assertEqual(rev1, branch.get_rev_id(1))
51+ self.assertLength(1, [call for call in self.hpss_calls if
52+ call.call.method == verb])
53+
54
55 class TestRepositoryIsShared(TestRemoteRepository):
56

Subscribers

People subscribed via source and target branches