Merge lp:~nataliabidart/ubuntuone-client/improve-error-handling into lp:ubuntuone-client

[Natalia Bidart]

Unified server error handling for check_version, set_capabilities, and authenticate.

Unified server error handling for check_version, set_capabilities, and authenticate.

This is a big branh but most of the diff lines are test cases added for existing functionality that had to be refactored, and had no test asserting over its functionality.

[John Lenton (chipaca)]

Is this a fix for something that is going into Lucid? If so, please link it. If not, I'm afraid it would be a lot easier if we merge it after the freeze.

review: Needs Information

[John Lenton (chipaca)]

OK, brown paper bag for me. I promise I *looked* for the bugs.
Getting more coffee, now.

[John Lenton (chipaca)]

review: Abstain

[John Lenton (chipaca)]

I like it.
I do have nits to pick:

* with the class called A:

    class A(object):
        """Do nothing."""
        pass

it isn't that it's hard to understand what the class does :) In this case, I think maybe the docstring should explain the intent a little more?

* a question: isn't using inlineCallbacks for a method that just calls one deferred-returning method, overkill?

* There's a pattern in the code that goes like

  kwargs = dict( ... a dict ... )
  yield self.foo(**kwargs)

without any real reason. That is a rather silly use of kwargs.

None of these are Need Fixings. On the whole, a lovely refactoring.

review: Approve

[Natalia Bidart (nataliabidart)]

> I like it.
> I do have nits to pick:
>
> * with the class called A:
>
> class A(object):
> """Do nothing."""
> pass
>
> it isn't that it's hard to understand what the class does :) In this case, I
> think maybe the docstring should explain the intent a little more?

Improved the docstring and renamed to SillyClass for the sake of explicitness.

> * a question: isn't using inlineCallbacks for a method that just calls one
> deferred-returning method, overkill?

Actually, I don't know. I preferred leave that way "to maintain" the "shape" of the former implementation. I'm changing check_versionto avoid using it.

> * There's a pattern in the code that goes like
>
> kwargs = dict( ... a dict ... )
> yield self.foo(**kwargs)
>
> without any real reason. That is a rather silly use of kwargs.

The real reason is that I find this approach the best way to pass a lot of parameters to a function and keep that call readable and within 80 columns. I'm not sure how to change it and keep it with those properties.

> None of these are Need Fixings. On the whole, a lovely refactoring.

Thank you very much for the input, and the review!

[lp:~nataliabidart/ubuntuone-client/improve-error-handling]

494. By Natalia Bidart on 2010-04-12

Applying review tips frm Chipaca.

495. By Natalia Bidart on 2010-04-12

Merged trunk in.

496. By Natalia Bidart on 2010-04-12

Removing unused ().

[Facundo Batista (facundo)]

It's ok!

review: Approve

[lp:~nataliabidart/ubuntuone-client/improve-error-handling]

497. By Natalia Bidart on 2010-04-12

Adding improvements suggested by facundobatista.

498. By Natalia Bidart on 2010-04-12

Fixing typos.

=== modified file 'contrib/testing/testcase.py'
--- contrib/testing/testcase.py	2010-03-15 19:18:32 +0000
+++ contrib/testing/testcase.py	2010-04-12 13:34:34 +0000
@@ -651,14 +651,10 @@
         self.events.append((event_name, args, kwargs))
 
 
-def noop(*args, **kwargs):
-    """No op."""
-
-
 class DummyClass(object):
     """Dummy class, does nothing."""
 
     def __getattribute__(self, name):
         """Any attribute is a no-op."""
-        return noop
+        return lambda *args, **kwargs: None
 

=== modified file 'tests/syncdaemon/test_action_queue.py'
--- tests/syncdaemon/test_action_queue.py	2010-04-09 05:12:37 +0000
+++ tests/syncdaemon/test_action_queue.py	2010-04-12 13:34:34 +0000
@@ -37,10 +37,10 @@
 from twisted.web import server
 
 from contrib.testing.testcase import (
-    BaseTwistedTestCase, MementoHandler
+    BaseTwistedTestCase, MementoHandler, DummyClass
 )
 
-from ubuntuone.storageprotocol import client, volumes
+from ubuntuone.storageprotocol import client, errors, protocol_pb2, volumes
 from ubuntuone.syncdaemon import states
 from ubuntuone.syncdaemon.dbus_interface import DBusInterface
 from ubuntuone.syncdaemon.main import Main
@@ -163,7 +163,7 @@
         self.main.event_q.push = keep_a_copy(self.main.event_q.push)
 
         self.handler = MementoHandler()
-        self.handler.setLevel(logging.ERROR)
+        self.handler.setLevel(logging.INFO)
         logging.getLogger('ubuntuone.SyncDaemon').addHandler(self.handler)
 
         # fake local rescan call to not be executed
@@ -1439,16 +1439,491 @@
             """Fake get_root."""
             d.callback(mdid)
 
-        class FakeClient(object):
-            """Fake Client"""
-            def __getattribute__(self, attr):
-                """Return a noop."""
-                return lambda *args, **kwargs: None
-
-        self.action_queue.client = FakeClient()
+        self.action_queue.client = DummyClass()
         self.action_queue.get_root = get_root
         self.action_queue.server_rescan('foo', lambda: list())
         def check(result):
             self.assertEquals('foo', result)
         d.addCallback(check)
         return d
+
+
+class SpecificException(Exception):
+    """The specific exception."""
+
+
+class SillyClass(object):
+    """Silly class that accepts the set of any attribute.
+
+    We can't use object() directly, since its raises AttributeError.
+
+    """
+
+
+class ErrorHandlingTestCase(BasicTestCase):
+    """Error handling tests for ActionQueue."""
+
+    def setUp(self):
+        """Init."""
+        BasicTestCase.setUp(self)
+
+        self.called = False
+        self.action_queue.client = SillyClass()
+        self.action_queue.deferred = defer.Deferred()
+        self.patch(self.main, 'restart', lambda: None)
+
+        self.main.start()
+
+    def fail_please(self, an_exception):
+        """Raise the given exception."""
+        def inner(*args, **kwargs):
+            """A request to the server that fails."""
+            self.called = True
+            return defer.fail(an_exception)
+        return inner
+
+    def succeed_please(self, result):
+        """Return the given result."""
+        def inner(*args, **kwargs):
+            """A request to the server that succeeds."""
+            self.called = True
+            return defer.succeed(result)
+        return inner
+
+    def mock_caps(self, accepted):
+        """Reply to query caps with False."""
+        def gset_caps(caps):
+            """get/set caps helper."""
+            req = SillyClass()
+            req.caps = caps
+            req.accepted = accepted
+            return defer.succeed(req)
+        return gset_caps
+
+    def test_valid_event(self):
+        """SYS_SERVER_ERROR is valid in EventQueue."""
+        event = 'SYS_SERVER_ERROR'
+        self.assertTrue(event in EVENTS)
+        self.assertEquals(('error',), EVENTS[event])
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_no_error(self):
+        """_send_request_and_handle_errors is correct when no error."""
+
+        event = 'SYS_SPECIFIC_OK'
+        EVENTS[event] = () # add event to the global valid events list
+        self.addCleanup(lambda: EVENTS.pop(event))
+
+        result = object()
+        request = self.succeed_please(result)
+        kwargs = dict(request=request, request_error=SpecificException,
+                      event_error='YADDA_YADDA', event_ok=event,
+                      args=(1, 2), kwargs={})
+        d = self.action_queue._send_request_and_handle_errors(**kwargs)
+        actual_result = yield d
+
+        self.assertTrue(self.called, 'the request was called')
+        self.assertEqual(actual_result, result)
+        self.assertEqual((event, (), {}),
+                         self.action_queue.event_queue.events[-1])
+
+        # assert over logging
+        self.assertEqual(1, len(self.handler.records))
+        self.assertIn(request.__name__, self.handler.records[0].message)
+        self.assertIn('OK', self.handler.records[0].message)
+
+        # assert internal deferred wasn't fired
+        self.assertFalse(self.action_queue.deferred.called)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_with_no_event_ok(self):
+        """_send_request_and_handle_errors does not push event if is None."""
+        original_events = self.action_queue.event_queue.events[:]
+
+        result = object()
+        request = self.succeed_please(result)
+        kwargs = dict(request=request, request_error=SpecificException,
+                      event_error='YADDA_YADDA', event_ok=None)
+        d = self.action_queue._send_request_and_handle_errors(**kwargs)
+        actual_result = yield d
+
+        self.assertTrue(self.called, 'the request was called')
+        self.assertEqual(actual_result, result)
+        self.assertEqual(original_events,
+                         self.action_queue.event_queue.events)
+
+        # assert over logging
+        self.assertEqual(1, len(self.handler.records))
+        self.assertIn(request.__name__, self.handler.records[0].message)
+        self.assertIn('OK', self.handler.records[0].message)
+
+        # assert internal deferred wasn't fired
+        self.assertFalse(self.action_queue.deferred.called)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_valid_error(self):
+        """_send_request_and_handle_errors is correct when expected error."""
+
+        event = 'SYS_SPECIFIC_ERROR'
+        EVENTS[event] = ('error',) # add event to the global valid events list
+        self.addCleanup(lambda: EVENTS.pop(event))
+
+        exc = SpecificException('The request failed! please be happy.')
+        request = self.fail_please(exc)
+        kwargs = dict(request=request, request_error=SpecificException,
+                      event_error=event, event_ok='YADDA_YADDA')
+        d = self.action_queue._send_request_and_handle_errors(**kwargs)
+        yield d
+
+        self.assertTrue(self.called, 'the request was called')
+        self.assertEqual((event, (), {'error': str(exc)}),
+                         self.action_queue.event_queue.events[-1])
+
+        # assert over logging
+        self.assertEqual(1, len(self.handler.records))
+        self.assertIn(request.__name__, self.handler.records[0].message)
+        self.assertIn(str(exc), self.handler.records[0].message)
+
+        # assert internal deferred was fired
+        self.assertTrue(self.action_queue.deferred.called)
+        self.assertIsInstance(self.action_queue.deferred.result, Failure)
+        self.assertEqual(self.action_queue.deferred.result.value, exc)
+
+    @defer.inlineCallbacks
+    def assert_send_request_and_handle_errors_on_server_error(self, serr):
+        """_send_request_and_handle_errors is correct when server error."""
+        # XXX: we need to replace this list with and exception list
+        # once bug #557718 is resolved
+        msg = protocol_pb2.Message()
+        msg.type = protocol_pb2.Message.ERROR
+        msg.error.type = serr
+        msg.error.comment = 'Error message for %s.' % serr
+        exc = errors.error_to_exception(serr)(request=None, message=msg)
+
+        request = self.fail_please(exc)
+        kwargs = dict(request=request, request_error=SpecificException,
+                      event_error='BAR', event_ok='FOO')
+        d = self.action_queue._send_request_and_handle_errors(**kwargs)
+        yield d
+
+        self.assertEqual(( 'SYS_SERVER_ERROR', (), {'error': str(exc)}),
+                     self.action_queue.event_queue.events[-1])
+
+        # assert over logging
+        self.assertTrue(len(self.handler.records) > 0)
+        self.assertIn(request.__name__, self.handler.records[-1].message)
+        self.assertIn(str(exc), self.handler.records[-1].message)
+
+        # assert internal deferred was fired
+        self.assertTrue(self.action_queue.deferred.called)
+        self.assertIsInstance(self.action_queue.deferred.result, Failure)
+        self.assertEqual(self.action_queue.deferred.result.value, exc)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_try_again(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.TRY_AGAIN
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_internal_error(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.INTERNAL_ERROR
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_protocol_error(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.PROTOCOL_ERROR
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_unsupported_version(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.UNSUPPORTED_VERSION
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_authetication_failed(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.AUTHENTICATION_FAILED
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_no_permission(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.NO_PERMISSION
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_already_exists(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.ALREADY_EXISTS
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_does_not_exist(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.DOES_NOT_EXIST
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_not_a_dir(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.NOT_A_DIRECTORY
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_not_empty(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.NOT_EMPTY
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_not_available(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.NOT_AVAILABLE
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_upload_in_porgress(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.UPLOAD_IN_PROGRESS
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_upload_corrupt(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.UPLOAD_CORRUPT
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_upload_canceled(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.UPLOAD_CANCELED
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_conflict(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.CONFLICT
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_quota_exceeded(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.QUOTA_EXCEEDED
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_invalid_filename(self):
+        """_send_request_and_handle_errors is correct when server error."""
+        serr = protocol_pb2.Error.INVALID_FILENAME
+        yield self.assert_send_request_and_handle_errors_on_server_error(serr)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_unknown_error(self):
+        """_send_request_and_handle_errors is correct when unknown error."""
+        # XXX: we need to replace this list with and exception list
+        # once bug #557718 is resolved
+        serr = protocol_pb2.Error.AUTHENTICATION_REQUIRED
+        msg = protocol_pb2.Message()
+        msg.type = protocol_pb2.Message.ERROR
+        msg.error.type = serr
+        msg.error.comment = 'Error message for %s.' % serr
+        exc = errors.error_to_exception(serr)(request=None, message=msg)
+
+        request = self.fail_please(exc)
+        kwargs = dict(request=request, request_error=SpecificException,
+                  event_error='BAR', event_ok='FOO')
+        d = self.action_queue._send_request_and_handle_errors(**kwargs)
+        yield d
+
+        self.assertIn(('SYS_UNKNOWN_ERROR', (), {}),
+                      self.action_queue.event_queue.events)
+
+        # assert over logging
+        self.assertEqual(1, len(self.handler.records))
+        self.assertIn(request.__name__, self.handler.records[0].message)
+        self.assertIn(str(exc), self.handler.records[0].message)
+
+        # assert internal deferred was fired
+        self.assertTrue(self.action_queue.deferred.called)
+        self.assertIsInstance(self.action_queue.deferred.result, Failure)
+        self.assertEqual(self.action_queue.deferred.result.value, exc)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_no_protocol_error(self):
+        """_send_request_and_handle_errors is correct when no-protocol error."""
+
+        event = 'SYS_UNKNOWN_ERROR'
+        error_msg = 'Error message for any Exception.'
+        exc = Exception(error_msg)
+        request = self.fail_please(exc)
+        kwargs = dict(request=request, request_error=SpecificException,
+                      event_error='BAR', event_ok='FOO')
+        d = self.action_queue._send_request_and_handle_errors(**kwargs)
+        yield d
+
+        self.assertIn((event, (), {}),
+                      self.action_queue.event_queue.events)
+
+        # assert over logging
+        self.assertEqual(1, len(self.handler.records))
+        self.assertIn(request.__name__, self.handler.records[0].message)
+        self.assertIn(str(exc), self.handler.records[0].message)
+
+        # assert internal deferred was fired
+        self.assertTrue(self.action_queue.deferred.called)
+        self.assertIsInstance(self.action_queue.deferred.result, Failure)
+        self.assertEqual(self.action_queue.deferred.result.value, exc)
+
+    @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_on_client_mismatch(self):
+        """_send_request_and_handle_errors is correct when client mismatch."""
+
+        def change_client(*args, **kwargs):
+            """Change AQ's client while doing the request."""
+            self.action_queue.client = object()
+
+        self.action_queue.event_queue.events = [] # event cleanup
+        kwargs = dict(request=change_client, request_error=SpecificException,
+                      event_error='BAR', event_ok='FOO')
+        d = self.action_queue._send_request_and_handle_errors(**kwargs)
+        yield d
+
+        self.assertEqual([], self.action_queue.event_queue.events)
+
+        # assert over logging
+        self.assertEqual(1, len(self.handler.records))
+        self.assertIn(change_client.__name__, self.handler.records[0].message)
+        self.assertIn('Client mismatch', self.handler.records[0].message)
+
+        # assert internal deferred wasn't fired
+        self.assertFalse(self.action_queue.deferred.called)
+
+    @defer.inlineCallbacks
+    def test_check_version_when_unsupported_version_exception(self):
+        """Test error handling after UnsupportedVersionError."""
+        # raise a UnsupportedVersionError
+        msg = protocol_pb2.Message()
+        msg.type = protocol_pb2.Message.ERROR
+        msg.error.type = protocol_pb2.Error.UNSUPPORTED_VERSION
+        msg.error.comment = 'This is a funny comment.'
+        exc = errors.UnsupportedVersionError(request=None, message=msg)
+
+        self.action_queue.client.protocol_version = self.fail_please(exc)
+        yield self.action_queue.check_version()
+        event = ('SYS_PROTOCOL_VERSION_ERROR', (), {'error': str(exc)})
+        self.assertEqual(event, self.action_queue.event_queue.events[-1])
+
+    @defer.inlineCallbacks
+    def test_set_capabilities_when_query_caps_not_accepted(self):
+        """Test error handling when the query caps are not accepeted."""
+
+        # query_caps returns False
+        self.action_queue.client.query_caps = self.mock_caps(accepted=False)
+
+        yield self.action_queue.set_capabilities(caps=None)
+        msg = "The server doesn't have the requested capabilities"
+        event = ('SYS_SET_CAPABILITIES_ERROR', (), {'error': msg})
+        self.assertEqual(event, self.action_queue.event_queue.events[-1])
+        self.assertNotIn(('SYS_SET_CAPABILITIES_OK', (), {}),
+                          self.action_queue.event_queue.events)
+
+        # assert internal deferred was fired
+        self.assertTrue(self.action_queue.deferred.called)
+        self.assertIsInstance(self.action_queue.deferred.result, Failure)
+        self.assertIsInstance(self.action_queue.deferred.result.value,
+                              StandardError)
+        self.assertEqual(str(self.action_queue.deferred.result.value), msg)
+
+    @defer.inlineCallbacks
+    def test_set_capabilities_when_set_caps_not_accepted(self):
+        """Test error handling when the query caps are not accepeted."""
+
+        # query_caps returns True and set_caps returns False
+        self.action_queue.client.query_caps = self.mock_caps(accepted=True)
+        self.action_queue.client.set_caps = self.mock_caps(accepted=False)
+
+        caps = 'very difficult cap'
+        yield self.action_queue.set_capabilities(caps=caps)
+        msg = "The server denied setting '%s' capabilities" % caps
+        event = ('SYS_SET_CAPABILITIES_ERROR', (), {'error': msg})
+        self.assertEqual(event, self.action_queue.event_queue.events[-1])
+        self.assertNotIn(('SYS_SET_CAPABILITIES_OK', (), {}),
+                          self.action_queue.event_queue.events)
+
+        # assert internal deferred was fired
+        self.assertTrue(self.action_queue.deferred.called)
+        self.assertIsInstance(self.action_queue.deferred.result, Failure)
+        self.assertIsInstance(self.action_queue.deferred.result.value,
+                              StandardError)
+        self.assertEqual(str(self.action_queue.deferred.result.value), msg)
+
+    @defer.inlineCallbacks
+    def test_set_capabilities_when_client_is_none(self):
+        """Test error handling when the client is None."""
+
+        self.action_queue.client = None
+
+        yield self.action_queue.set_capabilities(caps=None)
+        msg = "'NoneType' object has no attribute 'query_caps'"
+        event = ('SYS_SET_CAPABILITIES_ERROR', (), {'error': msg})
+        self.assertEqual(event, self.action_queue.event_queue.events[-1])
+        self.assertNotIn(('SYS_SET_CAPABILITIES_OK', (), {}),
+                          self.action_queue.event_queue.events)
+
+        # assert internal deferred was fired
+        self.assertTrue(self.action_queue.deferred.called)
+        self.assertIsInstance(self.action_queue.deferred.result, Failure)
+        self.assertIsInstance(self.action_queue.deferred.result.value,
+                              StandardError)
+        self.assertEqual(str(self.action_queue.deferred.result.value), msg)
+
+    @defer.inlineCallbacks
+    def test_set_capabilities_when_set_caps_is_accepted(self):
+        """Test error handling when the query caps are not accepeted."""
+
+        # query_caps returns True and set_caps returns True
+        self.action_queue.client.query_caps = self.mock_caps(accepted=True)
+        self.action_queue.client.set_caps = self.mock_caps(accepted=True)
+
+        yield self.action_queue.set_capabilities(caps=None)
+        event = ('SYS_SET_CAPABILITIES_OK', (), {})
+        self.assertEqual(event, self.action_queue.event_queue.events[-1])
+
+        # assert internal deferred wasn't fired
+        self.assertFalse(self.action_queue.deferred.called)
+
+    @defer.inlineCallbacks
+    def test_authenticate_when_authenticated(self):
+        """Test error handling after authenticate with no error."""
+
+        self.action_queue.client.oauth_authenticate = \
+            self.succeed_please(result=self.action_queue.client)
+        yield self.action_queue.authenticate(oauth_consumer=object())
+        event = ('SYS_AUTH_OK', (), {})
+        self.assertEqual(event, self.action_queue.event_queue.events[-1])
+
+        # assert internal deferred was fired with the client as result
+        self.assertTrue(self.action_queue.deferred.called)
+        self.assertTrue(self.action_queue.deferred.result is self.action_queue.client)
+
+    @defer.inlineCallbacks
+    def test_authenticate_when_authentication_failed_exception(self):
+        """Test error handling after AuthenticationFailedError."""
+        # raise a AuthenticationFailedError
+        msg = protocol_pb2.Message()
+        msg.type = protocol_pb2.Message.ERROR
+        msg.error.type = protocol_pb2.Error.AUTHENTICATION_FAILED
+        msg.error.comment = 'This is a funny comment.'
+        exc = errors.AuthenticationFailedError(request=None, message=msg)
+
+        self.action_queue.client.oauth_authenticate = self.fail_please(exc)
+        yield self.action_queue.authenticate(oauth_consumer=object())
+        event = ('SYS_AUTH_ERROR', (), {'error': str(exc)})
+        self.assertEqual(event, self.action_queue.event_queue.events[-1])
+
+        # assert internal deferred was fired
+        self.assertTrue(self.action_queue.deferred.called)

=== modified file 'ubuntuone/syncdaemon/action_queue.py'
--- ubuntuone/syncdaemon/action_queue.py	2010-04-09 18:21:13 +0000
+++ ubuntuone/syncdaemon/action_queue.py	2010-04-12 13:34:34 +0000
@@ -48,7 +48,7 @@
 from twisted.python.failure import Failure, DefaultException
 
 from oauth import oauth
-from ubuntuone.storageprotocol import protocol_pb2, request
+from ubuntuone.storageprotocol import errors, protocol_pb2
 from ubuntuone.storageprotocol.client import (
     ThrottlingStorageClient, ThrottlingStorageClientFactory
 )
@@ -882,111 +882,119 @@
         logger.warning('Connection lost: %s', reason.getErrorMessage())
 
     @defer.inlineCallbacks
-    def check_version(self):
-        """
-        Check the client protocol version matches that of the
-        server. Call callback on success, errback on failure.
+    def _send_request_and_handle_errors(self, request, request_error,
+                                        event_error, event_ok,
+                                        args=(), kwargs={}):
+        """Send 'request' to the server, using params 'args' and 'kwargs'.
+
+        Expect 'request_error' as valid error, and push 'event_error' in that
+        case. Do generic error handling for the rest of the protocol errors.
+
         """
         # if the client changes while we're waiting, this message is
         # old news and should be discarded (the message would
         # typically be a failure: timeout or disconnect). So keep the
         # original client around for comparison.
         client = self.client
+        req_name = request.__name__
+        failure = None
+        result = None
         try:
-            yield client.protocol_version()
-        except Exception, e:
-            if client is self.client:
-                logger.error("Protocol version error.")
-                logger.debug('Traceback follows:\n\n' + traceback.format_exc())
-                if e.message == 'UNSUPPORTED_VERSION':
-                    self.event_queue.push('SYS_PROTOCOL_VERSION_ERROR',
-                                          error=e.message)
-                else:
-                    self.event_queue.push('SYS_UNKNOWN_ERROR')
-                # it looks like we won't be authenticating, so hook up the
-                # for-testing deferred now
-                self.deferred.callback(Failure(e))
-        else:
-            if client is self.client:
-                logger.info("Protocol version OK")
-                self.event_queue.push('SYS_PROTOCOL_VERSION_OK')
+            try:
+                result = yield request(*args, **kwargs)
+            finally:
+                # common handling for all cases
+                if client is not self.client:
+                    client_mismatch_msg = "Client mismatch while processing "\
+                                          "the request '%s', " \
+                                          "client (%r) is not self.client (%r)."
+                    logger.warning(client_mismatch_msg, req_name,
+                                   client, self.client)
+                    return
+        except request_error, failure:
+            self.event_queue.push(event_error, error=str(failure))
+        except errors.AuthenticationRequiredError, failure:
+            # we need to separate this case from the rest because an
+            # AuthenticationRequiredError is an StorageRequestError,
+            # and we treat it differently.
+            self.event_queue.push('SYS_UNKNOWN_ERROR')
+        except errors.StorageRequestError, failure:
+            self.event_queue.push('SYS_SERVER_ERROR', error=str(failure))
+        except Exception, failure:
+            self.event_queue.push('SYS_UNKNOWN_ERROR')
+        else:
+            logger.info("The request '%s' finished OK.", req_name)
+            if event_ok is not None:
+                self.event_queue.push(event_ok)
+
+        if failure is not None:
+            logger.error("The request '%s' failed with the error:\n\n%s\n",
+                         req_name, failure)
+            # it looks like we won't be authenticating, so hook up the
+            # for-testing deferred now
+            self.deferred.callback(Failure(failure))
+        else:
+            defer.returnValue(result)
+
+    def check_version(self):
+        """Check if the client protocol version matches that of the server."""
+        check_version_d = self._send_request_and_handle_errors(
+            request=self.client.protocol_version,
+            request_error=errors.UnsupportedVersionError,
+            event_error='SYS_PROTOCOL_VERSION_ERROR',
+            event_ok='SYS_PROTOCOL_VERSION_OK'
+        )
+        return check_version_d
 
     @defer.inlineCallbacks
     def set_capabilities(self, caps):
-        """Set the capabilities with the server"""
-        client = self.client
-        is_failed = None
-        try:
-            req = (yield client.query_caps(caps))
-        except Exception, e:
-            is_failed = e
-        else:
+        """Set the capabilities with the server."""
+
+        @defer.inlineCallbacks
+        def caps_raising_if_not_accepted(capability_method, caps, msg):
+            client_caps = getattr(self.client, capability_method)
+            req = yield client_caps(caps)
             if not req.accepted:
-                is_failed = StandardError("The server doesn't have"
-                                          " the requested capabilities")
-
-        if client is self.client:
-            if is_failed is not None:
-                logger.error("Capabilities query failed: %s" % is_failed)
-                # Push the error to the event queue,
-                self.event_queue.push('SYS_SET_CAPABILITIES_ERROR',
-                                      error=is_failed.message)
-                # it looks like we won't be authenticating, so hook up the
-                # for-testing deferred now
-                self.deferred.callback(Failure(is_failed))
-                return
-
-            is_failed = None
-            try:
-                req = (yield client.set_caps(caps))
-            except Exception, e:
-                if client is self.client:
-                    is_failed = e
-            else:
-                if client is self.client:
-                    if req.accepted:
-                        self.event_queue.push('SYS_SET_CAPABILITIES_OK')
-                        defer.returnValue(self.client)
-                    else:
-                        is_failed = StandardError("The server denied setting "
-                                                  "%s capabilities" % req.caps)
-            if is_failed is not None:
-                logger.error("Capabilities set failed: %s", is_failed)
-                # Push the error to the event queue,
-                self.event_queue.push('SYS_SET_CAPABILITIES_ERROR',
-                                      error=is_failed.message)
-                # it looks like we won't be authenticating, so hook up the
-                # for-testing deferred now
-                self.deferred.callback(Failure(is_failed))
+                raise StandardError(msg)
+
+        error_msg = "The server doesn't have the requested capabilities"
+        query_caps_d = self._send_request_and_handle_errors(
+            request=caps_raising_if_not_accepted,
+            request_error=StandardError,
+            event_error='SYS_SET_CAPABILITIES_ERROR',
+            event_ok=None,
+            args=('query_caps', caps, error_msg)
+        )
+        yield query_caps_d
+
+        if self.deferred.called:
+            # the query_caps call finished with failure
+            return
+
+        error_msg = "The server denied setting '%s' capabilities" % caps
+        set_caps_d = self._send_request_and_handle_errors(
+            request=caps_raising_if_not_accepted,
+            request_error=StandardError,
+            event_error='SYS_SET_CAPABILITIES_ERROR',
+            event_ok='SYS_SET_CAPABILITIES_OK',
+            args=('set_caps', caps, error_msg)
+        )
+        yield set_caps_d
 
     @defer.inlineCallbacks
     def authenticate(self, oauth_consumer):
-        client = self.client
-        try:
-            yield client.oauth_authenticate(oauth_consumer, self.token)
-        except request.StorageRequestError, e:
-            if client is not self.client:
-                return
-            if e.error_message.error.type == \
-                                    protocol_pb2.Error.AUTHENTICATION_FAILED:
-                logger.error("OAuth failed: %s", e)
-                self.event_queue.push('SYS_AUTH_ERROR', error=str(e))
-            else:
-                logger.error("StorageRequestError during OAuth: %s", e)
-                self.event_queue.push('SYS_UNKNOWN_ERROR')
-            self.deferred.callback(Failure(e))
-        except Exception, e:
-            if client is not self.client:
-                return
-            logger.error("Generic error during OAuth: %s", e)
-            self.event_queue.push('SYS_UNKNOWN_ERROR')
-            self.deferred.callback(Failure(e))
-        else:
-            if client is not self.client:
-                return
-            logger.info("Oauth OK")
-            self.event_queue.push('SYS_AUTH_OK')
-            self.deferred.callback(client)
+        """Authenticate the client against the server using oauth_consumer."""
+        authenticate_d = self._send_request_and_handle_errors(
+            request=self.client.oauth_authenticate,
+            request_error=errors.AuthenticationFailedError,
+            event_error='SYS_AUTH_ERROR', event_ok='SYS_AUTH_OK',
+            args=(oauth_consumer, self.token)
+        )
+        yield authenticate_d
+
+        if not self.deferred.called:
+            # callback the deferred if everything went ok
+            self.deferred.callback(self.client)
 
     @defer.inlineCallbacks
     def server_rescan(self, root_mdid, data_gen):

=== modified file 'ubuntuone/syncdaemon/event_queue.py'
--- ubuntuone/syncdaemon/event_queue.py	2010-04-08 21:32:57 +0000
+++ ubuntuone/syncdaemon/event_queue.py	2010-04-12 13:34:34 +0000
@@ -129,6 +129,7 @@
     'SYS_HANDSHAKE_TIMEOUT': (),
     'SYS_ROOT_RECEIVED': ('root_id',),
     'SYS_ROOT_MISMATCH': ('root_id', 'new_root_id'),
+    'SYS_SERVER_ERROR': ('error',),
 
     'VM_UDF_SUBSCRIBED': ('udf',),
     'VM_UDF_SUBSCRIBE_ERROR': ('udf_id', 'error'),