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
=== modified file 'bzrlib/lockdir.py'
--- bzrlib/lockdir.py 2009-07-27 05:24:02 +0000
+++ bzrlib/lockdir.py 2009-08-31 04:36:50 +0000
@@ -181,10 +181,16 @@
181181
182 self._report_function = note182 self._report_function = note
183183
184 def _url(self):
185 try:
186 url = self.transport.external_url()
187 except errors.InProcessTransport:
188 url = self.transport.base
189 return url
190
184 def __repr__(self):191 def __repr__(self):
185 return '%s(%s%s)' % (self.__class__.__name__,192 return '%s(%s%s)' % (self.__class__.__name__,
186 self.transport.base,193 self._url(), self.path)
187 self.path)
188194
189 is_held = property(lambda self: self._lock_held)195 is_held = property(lambda self: self._lock_held)
190196
@@ -521,7 +527,7 @@
521 if deadline_str is None:527 if deadline_str is None:
522 deadline_str = time.strftime('%H:%M:%S',528 deadline_str = time.strftime('%H:%M:%S',
523 time.localtime(deadline))529 time.localtime(deadline))
524 lock_url = self.transport.abspath(self.path)530 lock_url = self._url()
525 self._report_function('%s %s\n'531 self._report_function('%s %s\n'
526 '%s\n' # held by532 '%s\n' # held by
527 '%s\n' # locked ... ago533 '%s\n' # locked ... ago
528534
=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py 2009-08-30 21:34:42 +0000
+++ bzrlib/remote.py 2009-08-31 04:36:50 +0000
@@ -2771,7 +2771,13 @@
2771 elif err.error_verb == 'norepository':2771 elif err.error_verb == 'norepository':
2772 raise errors.NoRepositoryPresent(find('bzrdir'))2772 raise errors.NoRepositoryPresent(find('bzrdir'))
2773 elif err.error_verb == 'LockContention':2773 elif err.error_verb == 'LockContention':
2774 raise errors.LockContention('(remote lock)')2774 if len(err.error_args) >= 2:
2775 args = err.error_args[:2]
2776 else:
2777 # Older servers (pre 1.18) usually don't send any details with a
2778 # LockContention.
2779 args = '(remote lock)', ''
2780 raise errors.LockContention(*args)
2775 elif err.error_verb == 'UnlockableTransport':2781 elif err.error_verb == 'UnlockableTransport':
2776 raise errors.UnlockableTransport(find('bzrdir').root_transport)2782 raise errors.UnlockableTransport(find('bzrdir').root_transport)
2777 elif err.error_verb == 'LockFailed':2783 elif err.error_verb == 'LockFailed':
27782784
=== modified file 'bzrlib/smart/branch.py'
--- bzrlib/smart/branch.py 2009-07-27 04:32:56 +0000
+++ bzrlib/smart/branch.py 2009-08-31 04:36:50 +0000
@@ -61,7 +61,6 @@
61 repository locks. The lock will be released once the request is61 repository locks. The lock will be released once the request is
62 processed. The physical lock state won't be changed.62 processed. The physical lock state won't be changed.
63 """63 """
64 # XXX: write a test for LockContention
65 branch.repository.lock_write(token=repo_token)64 branch.repository.lock_write(token=repo_token)
66 try:65 try:
67 branch.lock_write(token=branch_token)66 branch.lock_write(token=branch_token)
@@ -298,8 +297,6 @@
298 finally:297 finally:
299 # this leaves the repository with 1 lock298 # this leaves the repository with 1 lock
300 branch.repository.unlock()299 branch.repository.unlock()
301 except errors.LockContention:
302 return FailedSmartServerResponse(('LockContention',))
303 except errors.TokenMismatch:300 except errors.TokenMismatch:
304 return FailedSmartServerResponse(('TokenMismatch',))301 return FailedSmartServerResponse(('TokenMismatch',))
305 except errors.UnlockableTransport:302 except errors.UnlockableTransport:
306303
=== modified file 'bzrlib/smart/message.py'
--- bzrlib/smart/message.py 2009-07-28 22:27:04 +0000
+++ bzrlib/smart/message.py 2009-08-31 04:36:51 +0000
@@ -350,7 +350,13 @@
350 if error_name == 'UnknownMethod':350 if error_name == 'UnknownMethod':
351 raise errors.UnknownSmartMethod(error_args[0])351 raise errors.UnknownSmartMethod(error_args[0])
352 if error_name == 'LockContention':352 if error_name == 'LockContention':
353 raise errors.LockContention('(remote lock)')353 if len(error_args) >= 2:
354 args = error_args[:2]
355 else:
356 # Older servers (pre 1.18) usually don't send any details with a
357 # LockContention.
358 args = '(remote lock)', ''
359 raise errors.LockContention(*args)
354 elif error_name == 'LockFailed':360 elif error_name == 'LockFailed':
355 raise errors.LockFailed(*error_args[:2])361 raise errors.LockFailed(*error_args[:2])
356 elif error_name == 'FileExists':362 elif error_name == 'FileExists':
357363
=== modified file 'bzrlib/smart/request.py'
--- bzrlib/smart/request.py 2009-08-30 21:34:42 +0000
+++ bzrlib/smart/request.py 2009-08-31 04:36:51 +0000
@@ -401,7 +401,11 @@
401 elif isinstance(err, errors.TokenMismatch):401 elif isinstance(err, errors.TokenMismatch):
402 return ('TokenMismatch', err.given_token, err.lock_token)402 return ('TokenMismatch', err.given_token, err.lock_token)
403 elif isinstance(err, errors.LockContention):403 elif isinstance(err, errors.LockContention):
404<<<<<<< TREE
404 return ('LockContention',)405 return ('LockContention',)
406=======
407 return ('LockContention', repr(err.lock), err.msg)
408>>>>>>> MERGE-SOURCE
405 # Unserialisable error. Log it, and return a generic error409 # Unserialisable error. Log it, and return a generic error
406 trace.log_exception_quietly()410 trace.log_exception_quietly()
407 return ('error', str(err))411 return ('error', str(err))
408412
=== modified file 'bzrlib/tests/test_smart.py'
--- bzrlib/tests/test_smart.py 2009-08-27 22:17:35 +0000
+++ bzrlib/tests/test_smart.py 2009-08-31 04:36:51 +0000
@@ -47,6 +47,7 @@
47 SmartServerRequest,47 SmartServerRequest,
48 SmartServerResponse,48 SmartServerResponse,
49 SuccessfulSmartServerResponse,49 SuccessfulSmartServerResponse,
50 _translate_error,
50 )51 )
51from bzrlib.tests import (52from bzrlib.tests import (
52 split_suite_by_re,53 split_suite_by_re,
@@ -970,9 +971,10 @@
970 branch_token = branch.lock_write()971 branch_token = branch.lock_write()
971 branch.leave_lock_in_place()972 branch.leave_lock_in_place()
972 branch.unlock()973 branch.unlock()
973 response = request.execute('')974 # Executing the request raises LockContention.
974 self.assertEqual(975 err = self.assertRaises(errors.LockContention, request.execute, '')
975 SmartServerResponse(('LockContention',)), response)976 # The request handler will translate exceptions from execute
977 self.assertEqual('LockContention', _translate_error(err)[0])
976 # Cleanup978 # Cleanup
977 branch.lock_write(branch_token)979 branch.lock_write(branch_token)
978 branch.dont_leave_lock_in_place()980 branch.dont_leave_lock_in_place()
@@ -1030,9 +1032,10 @@
1030 repo_token = repo.lock_write()1032 repo_token = repo.lock_write()
1031 repo.leave_lock_in_place()1033 repo.leave_lock_in_place()
1032 repo.unlock()1034 repo.unlock()
1033 response = request.execute('')1035 # Executing the request raises LockContention.
1034 self.assertEqual(1036 err = self.assertRaises(errors.LockContention, request.execute, '')
1035 SmartServerResponse(('LockContention',)), response)1037 # The request handler will translate exceptions from execute
1038 self.assertEqual('LockContention', _translate_error(err)[0])
1036 # Cleanup1039 # Cleanup
1037 repo.lock_write(repo_token)1040 repo.lock_write(repo_token)
1038 repo.dont_leave_lock_in_place()1041 repo.dont_leave_lock_in_place()
10391042
=== modified file 'bzrlib/tests/test_smart_request.py'
--- bzrlib/tests/test_smart_request.py 2009-07-27 02:11:25 +0000
+++ bzrlib/tests/test_smart_request.py 2009-08-31 04:36:51 +0000
@@ -158,6 +158,7 @@
158 self.assertTranslationEqual(158 self.assertTranslationEqual(
159 ('NoSuchFile', 'path'), errors.NoSuchFile('path'))159 ('NoSuchFile', 'path'), errors.NoSuchFile('path'))
160160
161<<<<<<< TREE
161 def test_LockContention(self):162 def test_LockContention(self):
162 # For now, LockContentions are always transmitted with no details.163 # For now, LockContentions are always transmitted with no details.
163 # Eventually they should include a relpath or url or something else to164 # Eventually they should include a relpath or url or something else to
@@ -165,12 +166,33 @@
165 self.assertTranslationEqual(166 self.assertTranslationEqual(
166 ('LockContention',), errors.LockContention('lock', 'msg'))167 ('LockContention',), errors.LockContention('lock', 'msg'))
167168
169=======
170>>>>>>> MERGE-SOURCE
168 def test_TokenMismatch(self):171 def test_TokenMismatch(self):
169 self.assertTranslationEqual(172 self.assertTranslationEqual(
170 ('TokenMismatch', 'some-token', 'actual-token'),173 ('TokenMismatch', 'some-token', 'actual-token'),
171 errors.TokenMismatch('some-token', 'actual-token'))174 errors.TokenMismatch('some-token', 'actual-token'))
172175
173176
177class TestRequestHanderErrorTranslationWithTransport(TestCaseWithMemoryTransport):
178 """Like TestRequestHanderErrorTranslation, but for tests needing a
179 transport.
180 """
181
182 def assertTranslationEqual(self, expected_tuple, error):
183 self.assertEqual(expected_tuple, request._translate_error(error))
184
185 def test_LockContention(self):
186 from bzrlib.lockdir import LockDir
187 lock_dir = LockDir(self.get_transport(), 'foo')
188 lock_dir.create()
189 exception = errors.LockContention(lock_dir, 'msg')
190 exception_repr = repr(exception.lock)
191 self.assertTranslationEqual(
192 ('LockContention', exception_repr, 'msg'),
193 errors.LockContention(lock_dir, 'msg'))
194
195
174class TestRequestJail(TestCaseWithMemoryTransport):196class TestRequestJail(TestCaseWithMemoryTransport):
175 197
176 def test_jail(self):198 def test_jail(self):