Merge lp:~nataliabidart/ubuntuone-client/server-rescan-handles-errors-from-server into lp:ubuntuone-client

[Natalia Bidart]

Refactored ActionQueue.server_rescan so generic error handling is made.

Refactored ActionQueue.server_rescan so generic error handling is made.

The erro handling is done usingthe generic helper added in former branches. Please note that the special case of firing or not the self.deferred should be removed once Bug #561736 is resolved.

[Facundo Batista (facundo)]

Like it!

review: Approve

[Guillermo Gonzalez (verterok)]

review: Approve

[John Lenton (chipaca)]

The attempt to merge lp:~nataliabidart/ubuntuone-client/server-rescan-handles-errors-from-server into lp:ubuntuone-client failed.Below is the output from the failed tests.

Using saved parent location: bzr+ssh://bazaar.launchpad.net/~ubuntuone-control-tower/ubuntuone-storage-protocol/trunk/
No revisions to pull.
/usr/bin/gnome-autogen.sh
checking for autoconf >= 2.53...
(B testing autoconf2.50... not found.
  testing autoconf... found 2.65
checking for automake >= 1.10...
(B testing automake-1.11... found 1.11.1
checking for libtool >= 1.5...
(B testing libtoolize... found 2.2.6b
checking for intltool >= 0.30...
(B testing intltoolize... found 0.41.0
checking for pkg-config >= 0.14.0...
(B testing pkg-config... found 0.22
Checking for required M4 macros...
(BChecking for forbidden M4 macros...
(BProcessing ./configure.ac
(BRunning libtoolize...
(Blibtoolize: putting auxiliary files in `.'.
libtoolize: copying file `./ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'.
libtoolize: copying file `m4/libtool.m4'
libtoolize: copying file `m4/ltoptions.m4'
libtoolize: copying file `m4/ltsugar.m4'
libtoolize: copying file `m4/ltversion.m4'
libtoolize: copying file `m4/lt~obsolete.m4'
Running intltoolize...
(BRunning aclocal-1.11...
(BRunning autoconf...
(BRunning autoheader...
(BRunning automake-1.11...
(BRunning ./configure --prefix=/home/john/canonical/ubuntuone-storage-protocol/trunk ...
(Bchecking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking for style of include used by make... GNU
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking dependency style of gcc... gcc3
checking for library containing strerror... none required
checking for gcc... (cached) gcc
checking whether we are using the GNU C compiler... (cached) yes
checking whether gcc accepts -g... (cached) yes
checking for gcc option to accept ISO C89... (cached) none needed
checking dependency style of gcc... (cached) gcc3
checking build system type... x86_64-unknown-linux-gnu
checking host system type... x86_64-unknown-linux-gnu
checking for a sed that does not truncate output... /bin/sed
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for fgrep... /bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length of comm...

[John Lenton (chipaca)]

The attempt to merge lp:~nataliabidart/ubuntuone-client/server-rescan-handles-errors-from-server into lp:ubuntuone-client failed.Below is the output from the failed tests.

Using saved parent location: bzr+ssh://bazaar.launchpad.net/~ubuntuone-control-tower/ubuntuone-storage-protocol/trunk/
No revisions to pull.
/usr/bin/gnome-autogen.sh
checking for autoconf >= 2.53...
(B testing autoconf2.50... not found.
  testing autoconf... found 2.65
checking for automake >= 1.10...
(B testing automake-1.11... found 1.11.1
checking for libtool >= 1.5...
(B testing libtoolize... found 2.2.6b
checking for intltool >= 0.30...
(B testing intltoolize... found 0.41.0
checking for pkg-config >= 0.14.0...
(B testing pkg-config... found 0.22
Checking for required M4 macros...
(BChecking for forbidden M4 macros...
(BProcessing ./configure.ac
(BRunning libtoolize...
(Blibtoolize: putting auxiliary files in `.'.
libtoolize: copying file `./ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'.
libtoolize: copying file `m4/libtool.m4'
libtoolize: copying file `m4/ltoptions.m4'
libtoolize: copying file `m4/ltsugar.m4'
libtoolize: copying file `m4/ltversion.m4'
libtoolize: copying file `m4/lt~obsolete.m4'
Running intltoolize...
(BRunning aclocal-1.11...
(BRunning autoconf...
(BRunning autoheader...
(BRunning automake-1.11...
(BRunning ./configure --with-protocol=/home/john/canonical/ubuntuone-storage-protocol/trunk ...
(Bchecking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking for style of include used by make... GNU
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking dependency style of gcc... gcc3
checking for library containing strerror... none required
checking for gcc... (cached) gcc
checking whether we are using the GNU C compiler... (cached) yes
checking whether gcc accepts -g... (cached) yes
checking for gcc option to accept ISO C89... (cached) none needed
checking dependency style of gcc... (cached) gcc3
checking build system type... x86_64-unknown-linux-gnu
checking host system type... x86_64-unknown-linux-gnu
checking for a sed that does not truncate output... /bin/sed
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for fgrep... /bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length ...

=== modified file 'tests/syncdaemon/test_action_queue.py'
--- tests/syncdaemon/test_action_queue.py	2010-04-12 15:55:43 +0000
+++ tests/syncdaemon/test_action_queue.py	2010-04-12 19:44:26 +0000
@@ -1935,6 +1935,25 @@
         self.assertFalse(self.action_queue.deferred.called)
 
     @defer.inlineCallbacks
+    def test_send_request_and_handle_errors_when_fire_deferred_false(self):
+        """_send_request_and_handle_errors doesn't fire the deferred."""
+
+        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!')
+        request = self.fail_please(exc)
+        kwargs = dict(request=request, request_error=SpecificException,
+                      event_error=event, event_ok='YADDA_YADDA',
+                      fire_deferred=False)
+        d = self.action_queue._send_request_and_handle_errors(**kwargs)
+        yield d
+
+        # 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
@@ -2059,3 +2078,74 @@
 
         # assert internal deferred was fired
         self.assertTrue(self.action_queue.deferred.called)
+
+    @defer.inlineCallbacks
+    def test_server_rescan_as_a_whole(self):
+        """Test error handling after server_rescan with no error."""
+
+        def faked_get_root(marker):
+            """Fake the action_queue.get_root."""
+            root_id=object()
+            self.action_queue.event_queue.push('SYS_ROOT_RECEIVED',
+                                               root_id=root_id)
+            return root_id
+
+        self.patch(self.action_queue, 'get_root', faked_get_root)
+
+        self.action_queue.client.query = \
+            self.succeed_please(result=self.action_queue.client)
+        yield self.action_queue.server_rescan(root_mdid=object(), data_gen=list)
+        event = ('SYS_SERVER_RESCAN_DONE', (), {})
+        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_server_rescan_when_get_root_fails(self):
+        """Test error handling after server_rescan when get_root fails."""
+
+        msg = protocol_pb2.Message()
+        msg.type = protocol_pb2.Message.ERROR
+        msg.error.type = protocol_pb2.Error.PROTOCOL_ERROR
+        msg.error.comment = 'get_root failed'
+        exc = errors.StorageRequestError(request=None, message=msg)
+        self.patch(self.action_queue, 'get_root', self.fail_please(exc))
+
+        self.action_queue.client.query = self.fail_please(NotImplementedError())
+
+        yield self.action_queue.server_rescan(root_mdid=object(), data_gen=list)
+
+        event = ('SYS_SERVER_RESCAN_DONE', (), {})
+        self.assertNotIn(event, self.action_queue.event_queue.events)
+
+        event = ('SYS_SERVER_ERROR', (), {'error': str(exc)})
+        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_server_rescan_when_query_fails(self):
+        """Test error handling after server_rescan when query fails."""
+
+        self.patch(self.action_queue, 'get_root',
+                   self.succeed_please(result=object()))
+
+        msg = protocol_pb2.Message()
+        msg.type = protocol_pb2.Message.ERROR
+        msg.error.type = protocol_pb2.Error.PROTOCOL_ERROR
+        msg.error.comment = 'query failed'
+        exc = errors.StorageRequestError(request=None, message=msg)
+        self.action_queue.client.query = self.fail_please(exc)
+
+        yield self.action_queue.server_rescan(root_mdid=object(), data_gen=list)
+
+        event = ('SYS_SERVER_RESCAN_DONE', (), {})
+        self.assertNotIn(event, self.action_queue.event_queue.events)
+
+        event = ('SYS_SERVER_ERROR', (), {'error': str(exc)})
+        self.assertEqual(event, self.action_queue.event_queue.events[-1])
+
+        # assert internal deferred wasn't fired
+        self.assertFalse(self.action_queue.deferred.called)

=== modified file 'ubuntuone/syncdaemon/action_queue.py'
--- ubuntuone/syncdaemon/action_queue.py	2010-04-12 15:55:43 +0000
+++ ubuntuone/syncdaemon/action_queue.py	2010-04-12 19:44:26 +0000
@@ -886,6 +886,7 @@
     @defer.inlineCallbacks
     def _send_request_and_handle_errors(self, request, request_error,
                                         event_error, event_ok,
+                                        fire_deferred=True,
                                         args=(), kwargs={}):
         """Send 'request' to the server, using params 'args' and 'kwargs'.
 
@@ -907,7 +908,7 @@
             finally:
                 # common handling for all cases
                 if client is not self.client:
-                    client_mismatch_msg = "Client mismatch while processing "\
+                    client_mismatch_msg = "Client mismatch while processing " \
                                           "the request '%s', " \
                                           "client (%r) is not self.client (%r)."
                     logger.warning(client_mismatch_msg, req_name,
@@ -932,9 +933,10 @@
         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))
+            if fire_deferred:
+                # 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)
 
@@ -998,31 +1000,37 @@
             # callback the deferred if everything went ok
             self.deferred.callback(self.client)
 
-    @defer.inlineCallbacks
     def server_rescan(self, root_mdid, data_gen):
         """Do the server rescan."""
-        client = self.client
-        yield self.get_root(root_mdid)
-        if client is not self.client:
-            return
-        data = data_gen()
-        logger.info("Server rescan: will query %d objects", len(data))
-        # we check we're going to actually log, because this could be expensive
-        if logger.isEnabledFor(TRACE):
-            for share, node, hash in data:
-                logger.trace("Server rescan: share: %r, node: %r, hash: %s",
-                             share or '/root/', node, hash)
-            logger.trace("Server rescan: all data shown")
-        yield client.query(data)
-        if client is not self.client:
-            return
-        logger.info("Server rescan: done")
-        self.event_queue.push('SYS_SERVER_RESCAN_DONE')
+
+        @defer.inlineCallbacks
+        def _get_root_and_query(root_mdid, data_gen):
+            """Get user's root and then query each element in data_gen()."""
+            yield self.get_root(root_mdid)
+            data = data_gen()
+            logger.info("Server rescan: will query %d objects", len(data))
+            # check we're going to actually log, because this could be expensive
+            if logger.isEnabledFor(TRACE):
+                for share, node, hash in data:
+                    logger.trace("Server rescan: share: %r, node: %r, hash: %s",
+                                 share or '/root/', node, hash)
+                logger.trace("Server rescan: all data shown")
+            yield self.client.query(data)
+
+        get_root_and_query_d = self._send_request_and_handle_errors(
+            request=_get_root_and_query,
+            request_error=None, event_error=None,
+            event_ok='SYS_SERVER_RESCAN_DONE', fire_deferred=False,
+            args=(root_mdid, data_gen)
+        )
+        return get_root_and_query_d
 
     def get_root(self, marker):
-        """
-        Get the user's root uuid. Use the uuid_map, so the caller can
-        use the marker in followup operations.
+        """Get the user's root uuid.
+
+        Use the uuid_map, so the caller can use the marker in followup
+        operations.
+
         """
         log = mklog(logger, 'get_root', '', marker, marker=marker)
         log.debug('starting')