Bazaar

Merge lp:~spiv/bzr/lockcontention-bugs into lp:~bzr/bzr/trunk-old

  1. lockcontention-bugs
  2. Merge into trunk-old
Proposed by Andrew Bennetts
Status: Rejected
Rejected by: Andrew Bennetts
Proposed branch: lp:~spiv/bzr/lockcontention-bugs
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 192 lines (has conflicts) 
Text conflict in bzrlib/smart/request.py
Text conflict in bzrlib/tests/test_smart_request.py
To merge this branch: bzr merge lp:~spiv/bzr/lockcontention-bugs
Reviewer Review Type Date Requested Status
Martin Pool Approve
Review via email: mp+9228@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

This patch does a couple of things:

 - fixes the generic server-side error translation function so that it can
   serialise real LockContention errors, which now tend to have their 'lock'
   attribute be a LockDir rather than a str.
 - simplifies the Branch.lock_write HPSS request implementation so that it uses
   that generic translation rather than its own, less informative one.
 - improves the client-side translation so that it will optionally use the lock
   repr returned over the wire if one is given. (Existing clients will
   unconditionally say "(remote lock)" even if the server gives more info.)
 - changes LockDir's repr and error message text to use
   transport.external_url(), where possible — especially over HPSS this is much
   more likely to be useful. It will still use .base if external_url raises
   InProcessTransport, as some url, even a memory:/// one, is probably more
   helpful than none at all.

Hmm, perhaps I should extend test_errors to explicitly construct LockContention
with both a LockDir (as happens in process) and with a str (as happens when
constructing one from an HPSS error response)...

The change to use external_url *might* help fix the Launchpad-bazaar bug where
ephemeral and internal lp-1234:/// URLs are shown to users of the codehosting
service, rather than something useful.

-Andrew.

Revision history for this message
Martin Pool (mbp) wrote :

This looks ok, though I do wonder if having a class that sometimes contains a real lock object and sometimes a repr will cause problems for code that catches this exception.

If we're only ever going to print it, why not just always use a string?

If we're going to use the object, passing a string may cause failures.

It may be that code never uses the object, but humans sometimes poke at it from debuggers and they can be relied upon to know the difference.

If we did require it to be an object, I suppose we could create some kind of stub for a remote lock that fulfils the protocol but refuses to do anything, but for now that'd be excessive.

A similar issue occurs for any error that holds an object...

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

On further reflection, I'm not so sure this is a step in the right direction. So I'm rejecting this merge proposal for now to get it off the queue of "ready to merge" things.

Unmerged revisions

4561. By Andrew Bennetts

Fix serialisation of LockContention via HPSS, improve deserialisation, and use external_url() when possible.

4560. By Canonical.com Patch Queue Manager <email address hidden>

(jam) Add AbsentContentFactory.get_bytes_as,
 which just raises a better error.

4559. By Canonical.com Patch Queue Manager <email address hidden>

(mbp) workaround for ftp servers without APPE

4558. By Canonical.com Patch Queue Manager <email address hidden>

(mbp) various UIFactory cleanups including bug 387717

4557. By Canonical.com Patch Queue Manager <email address hidden>

(robertc) Add interface enforcement for the behaviour of iter_changes
 with missing subtrees with explicit paths - the whole subtree
 is returned. (Robert Collins)

4556. By Canonical.com Patch Queue Manager <email address hidden>

(jam) Bug #375867,
 don't prompt for password if ssh host doesn't support password auth.

4555. By Canonical.com Patch Queue Manager <email address hidden>

(andrew) Fix minor KeyError bug in -Dhpss when logging requests for
 unregistered methods.

4554. By Canonical.com Patch Queue Manager <email address hidden>

(jml) Merge in changes from 1.17.

4553. By Canonical.com Patch Queue Manager <email address hidden>

(robertc) Add checks for inventory deltas which try to ensure that
 deltas that are not an exact fit are not applied. (Robert
 Collins, bug 397705, bug 367633)

4552. By Canonical.com Patch Queue Manager <email address hidden>

(andrew) Fix NameError when handling redirection loops in
 read_mergeable_from_transport.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/lockdir.py'
2--- bzrlib/lockdir.py 2009-07-27 05:24:02 +0000
3+++ bzrlib/lockdir.py 2009-08-31 04:36:50 +0000
4@@ -181,10 +181,16 @@
5
6 self._report_function = note
7
8+ def _url(self):
9+ try:
10+ url = self.transport.external_url()
11+ except errors.InProcessTransport:
12+ url = self.transport.base
13+ return url
14+
15 def __repr__(self):
16 return '%s(%s%s)' % (self.__class__.__name__,
17- self.transport.base,
18- self.path)
19+ self._url(), self.path)
20
21 is_held = property(lambda self: self._lock_held)
22
23@@ -521,7 +527,7 @@
24 if deadline_str is None:
25 deadline_str = time.strftime('%H:%M:%S',
26 time.localtime(deadline))
27- lock_url = self.transport.abspath(self.path)
28+ lock_url = self._url()
29 self._report_function('%s %s\n'
30 '%s\n' # held by
31 '%s\n' # locked ... ago
32
33=== modified file 'bzrlib/remote.py'
34--- bzrlib/remote.py 2009-08-30 21:34:42 +0000
35+++ bzrlib/remote.py 2009-08-31 04:36:50 +0000
36@@ -2771,7 +2771,13 @@
37 elif err.error_verb == 'norepository':
38 raise errors.NoRepositoryPresent(find('bzrdir'))
39 elif err.error_verb == 'LockContention':
40- raise errors.LockContention('(remote lock)')
41+ if len(err.error_args) >= 2:
42+ args = err.error_args[:2]
43+ else:
44+ # Older servers (pre 1.18) usually don't send any details with a
45+ # LockContention.
46+ args = '(remote lock)', ''
47+ raise errors.LockContention(*args)
48 elif err.error_verb == 'UnlockableTransport':
49 raise errors.UnlockableTransport(find('bzrdir').root_transport)
50 elif err.error_verb == 'LockFailed':
51
52=== modified file 'bzrlib/smart/branch.py'
53--- bzrlib/smart/branch.py 2009-07-27 04:32:56 +0000
54+++ bzrlib/smart/branch.py 2009-08-31 04:36:50 +0000
55@@ -61,7 +61,6 @@
56 repository locks. The lock will be released once the request is
57 processed. The physical lock state won't be changed.
58 """
59- # XXX: write a test for LockContention
60 branch.repository.lock_write(token=repo_token)
61 try:
62 branch.lock_write(token=branch_token)
63@@ -298,8 +297,6 @@
64 finally:
65 # this leaves the repository with 1 lock
66 branch.repository.unlock()
67- except errors.LockContention:
68- return FailedSmartServerResponse(('LockContention',))
69 except errors.TokenMismatch:
70 return FailedSmartServerResponse(('TokenMismatch',))
71 except errors.UnlockableTransport:
72
73=== modified file 'bzrlib/smart/message.py'
74--- bzrlib/smart/message.py 2009-07-28 22:27:04 +0000
75+++ bzrlib/smart/message.py 2009-08-31 04:36:51 +0000
76@@ -350,7 +350,13 @@
77 if error_name == 'UnknownMethod':
78 raise errors.UnknownSmartMethod(error_args[0])
79 if error_name == 'LockContention':
80- raise errors.LockContention('(remote lock)')
81+ if len(error_args) >= 2:
82+ args = error_args[:2]
83+ else:
84+ # Older servers (pre 1.18) usually don't send any details with a
85+ # LockContention.
86+ args = '(remote lock)', ''
87+ raise errors.LockContention(*args)
88 elif error_name == 'LockFailed':
89 raise errors.LockFailed(*error_args[:2])
90 elif error_name == 'FileExists':
91
92=== modified file 'bzrlib/smart/request.py'
93--- bzrlib/smart/request.py 2009-08-30 21:34:42 +0000
94+++ bzrlib/smart/request.py 2009-08-31 04:36:51 +0000
95@@ -401,7 +401,11 @@
96 elif isinstance(err, errors.TokenMismatch):
97 return ('TokenMismatch', err.given_token, err.lock_token)
98 elif isinstance(err, errors.LockContention):
99+<<<<<<< TREE
100 return ('LockContention',)
101+=======
102+ return ('LockContention', repr(err.lock), err.msg)
103+>>>>>>> MERGE-SOURCE
104 # Unserialisable error. Log it, and return a generic error
105 trace.log_exception_quietly()
106 return ('error', str(err))
107
108=== modified file 'bzrlib/tests/test_smart.py'
109--- bzrlib/tests/test_smart.py 2009-08-27 22:17:35 +0000
110+++ bzrlib/tests/test_smart.py 2009-08-31 04:36:51 +0000
111@@ -47,6 +47,7 @@
112 SmartServerRequest,
113 SmartServerResponse,
114 SuccessfulSmartServerResponse,
115+ _translate_error,
116 )
117 from bzrlib.tests import (
118 split_suite_by_re,
119@@ -970,9 +971,10 @@
120 branch_token = branch.lock_write()
121 branch.leave_lock_in_place()
122 branch.unlock()
123- response = request.execute('')
124- self.assertEqual(
125- SmartServerResponse(('LockContention',)), response)
126+ # Executing the request raises LockContention.
127+ err = self.assertRaises(errors.LockContention, request.execute, '')
128+ # The request handler will translate exceptions from execute
129+ self.assertEqual('LockContention', _translate_error(err)[0])
130 # Cleanup
131 branch.lock_write(branch_token)
132 branch.dont_leave_lock_in_place()
133@@ -1030,9 +1032,10 @@
134 repo_token = repo.lock_write()
135 repo.leave_lock_in_place()
136 repo.unlock()
137- response = request.execute('')
138- self.assertEqual(
139- SmartServerResponse(('LockContention',)), response)
140+ # Executing the request raises LockContention.
141+ err = self.assertRaises(errors.LockContention, request.execute, '')
142+ # The request handler will translate exceptions from execute
143+ self.assertEqual('LockContention', _translate_error(err)[0])
144 # Cleanup
145 repo.lock_write(repo_token)
146 repo.dont_leave_lock_in_place()
147
148=== modified file 'bzrlib/tests/test_smart_request.py'
149--- bzrlib/tests/test_smart_request.py 2009-07-27 02:11:25 +0000
150+++ bzrlib/tests/test_smart_request.py 2009-08-31 04:36:51 +0000
151@@ -158,6 +158,7 @@
152 self.assertTranslationEqual(
153 ('NoSuchFile', 'path'), errors.NoSuchFile('path'))
154
155+<<<<<<< TREE
156 def test_LockContention(self):
157 # For now, LockContentions are always transmitted with no details.
158 # Eventually they should include a relpath or url or something else to
159@@ -165,12 +166,33 @@
160 self.assertTranslationEqual(
161 ('LockContention',), errors.LockContention('lock', 'msg'))
162
163+=======
164+>>>>>>> MERGE-SOURCE
165 def test_TokenMismatch(self):
166 self.assertTranslationEqual(
167 ('TokenMismatch', 'some-token', 'actual-token'),
168 errors.TokenMismatch('some-token', 'actual-token'))
169
170
171+class TestRequestHanderErrorTranslationWithTransport(TestCaseWithMemoryTransport):
172+ """Like TestRequestHanderErrorTranslation, but for tests needing a
173+ transport.
174+ """
175+
176+ def assertTranslationEqual(self, expected_tuple, error):
177+ self.assertEqual(expected_tuple, request._translate_error(error))
178+
179+ def test_LockContention(self):
180+ from bzrlib.lockdir import LockDir
181+ lock_dir = LockDir(self.get_transport(), 'foo')
182+ lock_dir.create()
183+ exception = errors.LockContention(lock_dir, 'msg')
184+ exception_repr = repr(exception.lock)
185+ self.assertTranslationEqual(
186+ ('LockContention', exception_repr, 'msg'),
187+ errors.LockContention(lock_dir, 'msg'))
188+
189+
190 class TestRequestJail(TestCaseWithMemoryTransport):
191
192 def test_jail(self):