Merge lp:~gz/bzr/smarter_smart_error_reporting_722416_B into lp:bzr

[Martin Packman]

This is step 3 (really) in fixing bug 722416. The aim of the branch is to turn an remote response in the form `('MemoryError',)` into a useful message for the user. I'm pretty sure the code is incorrect or incomplete however.

I've got two main problems here. Firstly, there seem to be two different _translate_error functions for the local-side error interpretation. Secondly, I can only reproduce one of the codepaths from the bug reports in the tests.

The stack I can test is going through autopack as-like bug 602614, which uses the bzrlib.remote._translate_error function. However, I think the one I really need to patch is the bzrlib.smart.message._translate_error but I'm failing to write a test with a stack like the local-side one in bug 721598 as the important bits in test_remote seem to be Fake'd out. Help?

Note re-raising MemoryError locally isn't what we want to do, that's not the starving python. A new error class could be created though. Is there any additional 'advice' it would be useful to show to the user? Perhaps a suggestion to try the operation again locally or with 'nosmart+' instead?

[Andrew Bennetts (spiv)]

Martin [gz] wrote:
[...]
> This is step 3 (really) in fixing bug 722416. The aim of the branch is to turn
> an remote response in the form `('MemoryError',)` into a useful message for
> the user. I'm pretty sure the code is incorrect or incomplete however.
>
> I've got two main problems here. Firstly, there seem to be two different
> _translate_error functions for the local-side error interpretation. Secondly,
> I can only reproduce one of the codepaths from the bug reports in the tests.
>
> The stack I can test is going through autopack as-like bug 602614, which uses
> the bzrlib.remote._translate_error function. However, I think the one I really
> need to patch is the bzrlib.smart.message._translate_error but I'm failing to
> write a test with a stack like the local-side one in bug 721598 as the
> important bits in test_remote seem to be Fake'd out. Help?

Hmm. I don't think we have good unit tests for
bzrlib.smart.message._translate_error. Well, we do have
TestClientDecodingProtocolThree in test_smart_transport, which currently tests
the UnknownMethod and ErrorFromSmartServer paths there, but via the byte-level
details of the v3 protocol which you don't really want to care about for this.
I suspect the other paths are only exercised incidentally via the broader test
suite.

The intent was that message._translate_error would be for all the context-free
error translations, i.e. error translations that are independent of the request
that triggered them, such as UnknownMethod. Then the translations that want
access to the branch object or whatever belong in remote._translate_error. So
you're right that message._translate_error is probably the better place.

I'm not sure the distinction is particularly important, and looking at it with
relatively fresh eyes it has the drawback that the message._translate_error
function is never used by v2 (or v1, but it has a fixed set of error names
anyway), and so the cases other than UnknownMethod and ErrorFromSmartServer are
thus duplicated anyway. I'm also not sure that drawback matters much though,
although worryingly NoSuchFile has richer behaviour in remote._translate_error
that will never be triggered in v3 because message._translate_error will see it
first.

We have good unit tests I think that the protocol layer signals the presence of
an error to its caller, and from the other end test_remote does pretty good job
of unit testing remote._translate_error (at a glance I see
TestErrorTranslationSuccess is missing one or two newer errors like NotStacked).
But as you've noticed there's a gap for message._translate_error. I think the
ideal solution is to write the unit tests for that gap, but I don't want to
force you to do that just for this fix. Hmm :/

I'm currently inclined to say:

 * leave the UnknownMethod and ErrorFromSmartServer paths in
   message._translate_error: they probably do belong there, and are already well
   tested.
 * move the context-free translations into a new function, probably in remote.py
 * invoke that new function as part of remote._translate_error

This I think satisfies all the concerns adequately:

 * the context-free translations don't clutter up the more complex...

[Martin Packman (gz)]

> The intent was that message._translate_error would be for all the context-free
> error translations, i.e. error translations that are independent of the
> request
> that triggered them, such as UnknownMethod. Then the translations that want
> access to the branch object or whatever belong in remote._translate_error. So
> you're right that message._translate_error is probably the better place.

This helps, thanks. As a MemoryError could be raised from any command, it makes sense for it to be re-raised from the context-free translation function. However, see the ☃ question below.

> I'm currently inclined to say:
>
> * leave the UnknownMethod and ErrorFromSmartServer paths in
> message._translate_error: they probably do belong there, and are already
> well
> tested.
> * move the context-free translations into a new function, probably in
> remote.py
> * invoke that new function as part of remote._translate_error

I'll do this for now, and resist the urge to pointlessly rewrite in a more functional style.

> > Note re-raising MemoryError locally isn't what we want to do, that's not the
> > starving python. A new error class could be created though. Is there any
> > additional 'advice' it would be useful to show to the user? Perhaps a
> > suggestion to try the operation again locally or with 'nosmart+' instead?

Your thoughts in response to this much the same as mine, so will just add a little more text and leave at that.

> It's probably a good idea for the error to say which remote server, though.
> Various scripts, plugins, stacking configurations, etc, may have multiple
> connections active at one time, and the user probably wants to know which one
> had the problem :)

That's a good point. Though... ☃ asks doesn't that mean we do want context then? It also goes for the other general errors that are being re-raised, so perhaps a different approach might be helpful in the future.

[Martin Packman (gz)]

Sorry, bad diff in email again, launchpad hadn't finished updating the branch when it got sent out apparently, see the mp for the updates.

[Andrew Bennetts (spiv)]

Martin [gz] wrote:
[...]
> > It's probably a good idea for the error to say which remote server, though.
> > Various scripts, plugins, stacking configurations, etc, may have multiple
> > connections active at one time, and the user probably wants to know which one
> > had the problem :)
>
> That's a good point. Though... ☃ asks doesn't that mean we do want
> context then? It also goes for the other general errors that are being
> re-raised, so perhaps a different approach might be helpful in the
> future.

I suppose you're right! Although there's sort of different levels of
context...

I think part of the reason I had the distinct message._translate_error
vs. remote._translate_error functions was to keep knowledge of random
bzr model stuff out of the basic protocol. i.e. there's no reason why
the message layer, which is about taking simple structures off the wire
and turning them into events like “here's a request body” should know
anything about what NoSuchRevision means.

I don't think I thought through that particular refactoring carefully at
the time, and so conflated “doesn't need context stuff from remote.py”
with “message-level logic.” But it was a step towards that, and now
with your patches I think it's actually pretty right: the message layer
knows that an error is either UnknownMethodError, or else it is a
ErrorFromSmartServer for a higher layer to interpret, and that's it.

It's not really a goal, but if you like you can imagine a design
principle here as being “the smart protocol stuff could be used to write
useful servers and clients that have nothing to do with bzr.” So
bzrlib.smart.protocol, bzrlib.smart.message, bzrlib.smart.client and
most of bzrlib.smart.request for instance are pretty generic and on
nothing-to-do-with-the-rest-of-bzr side of the fence, while
bzrlib.remote and bzrlib.smart.bzrdir, etc, clearly belong to other
side.

That's a long winded way of saying: yes, I think you're right that the
medium or its URL or something probably belongs in the context, and that
the MemoryError translation (and possibly others) should use that.

And you're making me think that the no-context distinction isn't
important at all (now that I've figured out that it was the “belongs in
bzrlib.smart.message or not” distinction that I really cared about, even
though I had it a bit wrong).

-Andrew.

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2011-02-19 17:37:45 +0000
+++ bzrlib/remote.py	2011-03-02 20:20:52 +0000
@@ -2973,10 +2973,7 @@
                     'Missing key %r in context %r', key_err.args[0], context)
                 raise err
 
-    if err.error_verb == 'IncompatibleRepositories':
-        raise errors.IncompatibleRepositories(err.error_args[0],
-            err.error_args[1], err.error_args[2])
-    elif err.error_verb == 'NoSuchRevision':
+    if err.error_verb == 'NoSuchRevision':
         raise NoSuchRevision(find('branch'), err.error_args[0])
     elif err.error_verb == 'nosuchrevision':
         raise NoSuchRevision(find('repository'), err.error_args[0])
@@ -2989,22 +2986,12 @@
             detail=extra)
     elif err.error_verb == 'norepository':
         raise errors.NoRepositoryPresent(find('bzrdir'))
-    elif err.error_verb == 'LockContention':
-        raise errors.LockContention('(remote lock)')
     elif err.error_verb == 'UnlockableTransport':
         raise errors.UnlockableTransport(find('bzrdir').root_transport)
-    elif err.error_verb == 'LockFailed':
-        raise errors.LockFailed(err.error_args[0], err.error_args[1])
     elif err.error_verb == 'TokenMismatch':
         raise errors.TokenMismatch(find('token'), '(remote token)')
     elif err.error_verb == 'Diverged':
         raise errors.DivergedBranches(find('branch'), find('other_branch'))
-    elif err.error_verb == 'TipChangeRejected':
-        raise errors.TipChangeRejected(err.error_args[0].decode('utf8'))
-    elif err.error_verb == 'UnstackableBranchFormat':
-        raise errors.UnstackableBranchFormat(*err.error_args)
-    elif err.error_verb == 'UnstackableRepositoryFormat':
-        raise errors.UnstackableRepositoryFormat(*err.error_args)
     elif err.error_verb == 'NotStacked':
         raise errors.NotStacked(branch=find('branch'))
     elif err.error_verb == 'PermissionDenied':
@@ -3020,6 +3007,24 @@
     elif err.error_verb == 'NoSuchFile':
         path = get_path()
         raise errors.NoSuchFile(path)
+    _translate_error_without_context(err)
+
+
+def _translate_error_without_context(err):
+    """Translate any ErrorFromSmartServer values that don't require context"""
+    if err.error_verb == 'IncompatibleRepositories':
+        raise errors.IncompatibleRepositories(err.error_args[0],
+            err.error_args[1], err.error_args[2])
+    elif err.error_verb == 'LockContention':
+        raise errors.LockContention('(remote lock)')
+    elif err.error_verb == 'LockFailed':
+        raise errors.LockFailed(err.error_args[0], err.error_args[1])
+    elif err.error_verb == 'TipChangeRejected':
+        raise errors.TipChangeRejected(err.error_args[0].decode('utf8'))
+    elif err.error_verb == 'UnstackableBranchFormat':
+        raise errors.UnstackableBranchFormat(*err.error_args)
+    elif err.error_verb == 'UnstackableRepositoryFormat':
+        raise errors.UnstackableRepositoryFormat(*err.error_args)
     elif err.error_verb == 'FileExists':
         raise errors.FileExists(err.error_args[0])
     elif err.error_verb == 'DirectoryNotEmpty':
@@ -3044,4 +3049,6 @@
             raise UnicodeEncodeError(encoding, val, start, end, reason)
     elif err.error_verb == 'ReadOnlyError':
         raise errors.TransportNotPossible('readonly transport')
+    elif err.error_verb == 'MemoryError':
+        raise errors.BzrError("remote server out of memory")
     raise errors.UnknownErrorFromSmartServer(err)

=== modified file 'bzrlib/smart/message.py'
--- bzrlib/smart/message.py	2009-09-11 06:07:05 +0000
+++ bzrlib/smart/message.py	2011-03-02 20:20:52 +0000
@@ -303,7 +303,7 @@
             mutter('   result:   %r', self.args)
         if self.status == 'E':
             self._wait_for_response_end()
-            _translate_error(self.args)
+            _raise_smart_server_error(self.args)
         return tuple(self.args)
 
     def read_body_bytes(self, count=-1):
@@ -335,27 +335,17 @@
                 yield bytes_part
             self._read_more()
         if self._body_stream_status == 'E':
-            _translate_error(self._body_error_args)
+            _raise_smart_server_error(self._body_error_args)
 
     def cancel_read_body(self):
         self._wait_for_response_end()
 
 
-def _translate_error(error_tuple):
-    # Many exceptions need some state from the requestor to be properly
-    # translated (e.g. they need a branch object).  So this only translates a
-    # few errors, and the rest are turned into a generic ErrorFromSmartServer.
-    error_name = error_tuple[0]
-    error_args = error_tuple[1:]
-    if error_name == 'UnknownMethod':
-        raise errors.UnknownSmartMethod(error_args[0])
-    if error_name == 'LockContention':
-        raise errors.LockContention('(remote lock)')
-    elif error_name == 'LockFailed':
-        raise errors.LockFailed(*error_args[:2])
-    elif error_name == 'FileExists':
-        raise errors.FileExists(error_args[0])
-    elif error_name == 'NoSuchFile':
-        raise errors.NoSuchFile(error_args[0])
-    else:
-        raise errors.ErrorFromSmartServer(error_tuple)
+def _raise_smart_server_error(error_tuple):
+    """Raise exception based on tuple received from smart server
+
+    Specific error translation is handled by bzrlib.remote._translate_error
+    """
+    if error_tuple[0] == 'UnknownMethod':
+        raise errors.UnknownSmartMethod(error_tuple[1])
+    raise errors.ErrorFromSmartServer(error_tuple)

=== modified file 'bzrlib/smart/request.py'
--- bzrlib/smart/request.py	2010-11-26 06:31:54 +0000
+++ bzrlib/smart/request.py	2011-03-02 20:20:52 +0000
@@ -446,6 +446,9 @@
         return ('TokenMismatch', err.given_token, err.lock_token)
     elif isinstance(err, errors.LockContention):
         return ('LockContention',)
+    elif isinstance(err, MemoryError):
+        # GZ 2011-02-24: Copy bzrlib.trace -Dmem_dump functionality here?
+        return ('MemoryError',)
     # Unserialisable error.  Log it, and return a generic error
     trace.log_exception_quietly()
     return ('error', str(err))

=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py	2011-02-24 16:13:39 +0000
+++ bzrlib/tests/test_remote.py	2011-03-02 20:20:52 +0000
@@ -2775,6 +2775,16 @@
              ('pack collection autopack',)],
             client._calls)
 
+    def test_oom_error_reporting(self):
+        """An out-of-memory condition on the server is reported clearly"""
+        transport_path = 'quack'
+        repo, client = self.setup_fake_client_and_repository(transport_path)
+        client.add_expected_call(
+            'PackRepository.autopack', ('quack/',),
+            'error', ('MemoryError',))
+        err = self.assertRaises(errors.BzrError, repo.autopack)
+        self.assertContainsRe(str(err), "^remote server out of mem")
+
 
 class TestErrorTranslationBase(tests.TestCaseWithMemoryTransport):
     """Base class for unit tests for bzrlib.remote._translate_error."""
@@ -2853,6 +2863,13 @@
             detail='extra detail')
         self.assertEqual(expected_error, translated_error)
 
+    def test_norepository(self):
+        bzrdir = self.make_bzrdir('')
+        translated_error = self.translateTuple(('norepository',),
+            bzrdir=bzrdir)
+        expected_error = errors.NoRepositoryPresent(bzrdir)
+        self.assertEqual(expected_error, translated_error)
+
     def test_LockContention(self):
         translated_error = self.translateTuple(('LockContention',))
         expected_error = errors.LockContention('(remote lock)')
@@ -2886,6 +2903,12 @@
         expected_error = errors.DivergedBranches(branch, other_branch)
         self.assertEqual(expected_error, translated_error)
 
+    def test_NotStacked(self):
+        branch = self.make_branch('')
+        translated_error = self.translateTuple(('NotStacked',), branch=branch)
+        expected_error = errors.NotStacked(branch)
+        self.assertEqual(expected_error, translated_error)
+
     def test_ReadError_no_args(self):
         path = 'a path'
         translated_error = self.translateTuple(('ReadError',), path=path)
@@ -2907,7 +2930,8 @@
 
     def test_PermissionDenied_no_args(self):
         path = 'a path'
-        translated_error = self.translateTuple(('PermissionDenied',), path=path)
+        translated_error = self.translateTuple(('PermissionDenied',),
+            path=path)
         expected_error = errors.PermissionDenied(path)
         self.assertEqual(expected_error, translated_error)
 
@@ -2936,6 +2960,39 @@
         expected_error = errors.PermissionDenied(path, extra)
         self.assertEqual(expected_error, translated_error)
 
+    # GZ 2011-03-02: TODO test for PermissionDenied with non-ascii 'extra'
+
+    def test_NoSuchFile_context_path(self):
+        local_path = "local path"
+        translated_error = self.translateTuple(('ReadError', "remote path"),
+            path=local_path)
+        expected_error = errors.ReadError(local_path)
+        self.assertEqual(expected_error, translated_error)
+
+    def test_NoSuchFile_without_context(self):
+        remote_path = "remote path"
+        translated_error = self.translateTuple(('ReadError', remote_path))
+        expected_error = errors.ReadError(remote_path)
+        self.assertEqual(expected_error, translated_error)
+
+    def test_ReadOnlyError(self):
+        translated_error = self.translateTuple(('ReadOnlyError',))
+        expected_error = errors.TransportNotPossible("readonly transport")
+        self.assertEqual(expected_error, translated_error)
+
+    def test_MemoryError(self):
+        translated_error = self.translateTuple(('MemoryError',))
+        expected_error = errors.BzrError("remote server out of memory")
+        self.assertEqual(expected_error, translated_error)
+
+    def test_generic_IndexError(self):
+        err = errors.ErrorFromSmartServer(('error', "list index out of range"))
+        translated_error = self.translateErrorFromSmartServer(err)
+        expected_error = errors.UnknownErrorFromSmartServer(err)
+        self.assertEqual(expected_error, translated_error)
+
+    # GZ 2011-03-02: TODO test generic non-ascii error string
+
 
 class TestErrorTranslationRobustness(TestErrorTranslationBase):
     """Unit tests for bzrlib.remote._translate_error's robustness.

=== modified file 'bzrlib/tests/test_smart_request.py'
--- bzrlib/tests/test_smart_request.py	2010-07-13 19:02:12 +0000
+++ bzrlib/tests/test_smart_request.py	2011-03-02 20:20:52 +0000
@@ -172,6 +172,9 @@
             ('TokenMismatch', 'some-token', 'actual-token'),
             errors.TokenMismatch('some-token', 'actual-token'))
 
+    def test_MemoryError(self):
+        self.assertTranslationEqual(("MemoryError",), MemoryError())
+
 
 class TestRequestJail(TestCaseWithMemoryTransport):