Merge lp:~facundo/ubuntuone-client/sys-quota-exceeded-back into lp:ubuntuone-client

[Facundo Batista]

Bring SYS_QUOTA_EXCEEDED back (LP: #720797)

Bring SYS_QUOTA_EXCEEDED back

Commands now have a handle_retryable, a method they can use to receive a failure that does not make the command end, but to retry it.

The only one using it ATM is Upload, to send SYS_QUOTA_EXCEEDED.

Also, when testing this I noticed that if a command got stuck because of conditions, it never released the pathlock (even if cancelled!). So fixed that.

Tests included for everything.

[John O'Brien (jdobrien)]

I love proposals that have more tests than code and find & fix a few bugs on the way.

review: Approve

[Facundo Batista (facundo)]

Approving with one review

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

Attempt to merge into lp:ubuntuone-client failed due to conflicts:

text conflict in tests/syncdaemon/test_action_queue.py
text conflict in ubuntuone/syncdaemon/action_queue.py

[lp:~facundo/ubuntuone-client/sys-quota-exceeded-back]

906. By Facundo Batista on 2011-03-02

Merged trunk in

=== modified file 'tests/syncdaemon/test_action_queue.py'
--- tests/syncdaemon/test_action_queue.py	2011-03-02 14:24:13 +0000
+++ tests/syncdaemon/test_action_queue.py	2011-03-02 19:30:59 +0000
@@ -1936,10 +1936,11 @@
         self.assertEqual(called, [exc, 1])
 
     def test_run_enderr_retry(self):
-        """Command retried, don't call finish and retry."""
+        """Command retried, call the handle and retry."""
         called = []
-        self.cmd.finish = lambda: called.append(True)
-        self.cmd.handle_failure = lambda: called.append(True)
+        self.cmd.finish = lambda: called.append('should not')
+        self.cmd.handle_failure = lambda: called.append('should not')
+        self.cmd.handle_retryable = lambda f: called.append('ok')
         self.cmd.markers_resolved_deferred = defer.succeed(True)
         assert self.rq.active
 
@@ -1953,7 +1954,7 @@
 
         # run and check finish was not called
         self.cmd.run()
-        self.assertFalse(called)
+        self.assertEqual(called, ['ok'])
 
     def test_run_retry_on_commandpaused(self):
         """Command retried because of pausing."""
@@ -2048,6 +2049,7 @@
         # nothing called when no deferred
         assert self.cmd.wait_for_queue is None
         self.cmd.resume()
+        self.assertFalse(self.handler.check_debug('resuming'))
 
         # the deferred is triggered if there
         d = defer.Deferred()
@@ -2055,12 +2057,14 @@
         self.cmd.resume()
         self.assertIdentical(self.cmd.wait_for_queue, None)
         self.assertTrue(d.called)
+        self.assertTrue(self.handler.check_debug('resuming'))
 
     def test_check_conditions(self):
         """Trigger the deferred only if there."""
         # nothing called when no deferred
         assert self.cmd.wait_for_conditions is None
         self.cmd.check_conditions()
+        self.assertFalse(self.handler.check_debug('unblocking conditions'))
 
         # the deferred is triggered if there
         d = defer.Deferred()
@@ -2068,6 +2072,7 @@
         self.cmd.check_conditions()
         self.assertIdentical(self.cmd.wait_for_conditions, None)
         self.assertTrue(d.called)
+        self.assertTrue(self.handler.check_debug('unblocking conditions'))
 
     def test_cancel_works(self):
         """Do default cleaning."""
@@ -2079,6 +2084,14 @@
         self.assertEqual(called, [1, 2])
         self.assertTrue(self.cmd.cancelled)
 
+    def test_cancel_releases_conditions(self):
+        """Cancel unlocks the conditions deferred."""
+        self.cmd.finish = lambda: None # don't try to unqueue!
+        d = defer.Deferred()
+        self.cmd.wait_for_conditions = d
+        self.cmd.cancel()
+        self.assertTrue(d.called)
+
     def test_cancel_cancelled(self):
         """Don't do anything if command already cancelled."""
         called = []
@@ -3128,7 +3141,7 @@
         self.command.handle_failure(failure=failure)
         self.assertFalse(os.path.exists(self.command.tempfile.name))
 
-    def test_handle_failure_push_quota_exceeded_if_quota_exceeded_error(self):
+    def test_retryable_failure_push_quota_exceeded_if_that_error(self):
         """Test SYS_QUOTA_EXCEEDED is pushed on QuotaExceededError."""
         protocol_msg = protocol_pb2.Message()
         protocol_msg.type = protocol_pb2.Message.ERROR
@@ -3138,15 +3151,18 @@
         error = errors.QuotaExceededError("request", protocol_msg)
         failure = Failure(error)
 
-        self.command.handle_failure(failure=failure)
-        volume_id = self.command.share_id
-        kwargs = dict(share_id=volume_id, node_id='a_node_id',
-                      hash='yadda', error=str(error))
-        event1 = ('SYS_QUOTA_EXCEEDED',
-                  {'volume_id': volume_id, 'free_bytes': 1331564676})
-        event2 = ('AQ_UPLOAD_ERROR', kwargs)
-        self.assertTrue(event1 in self.command.action_queue.event_queue.events)
-        self.assertTrue(event2 in self.command.action_queue.event_queue.events)
+        self.command.handle_retryable(failure)
+        event = ('SYS_QUOTA_EXCEEDED', {'volume_id': self.command.share_id,
+                                        'free_bytes': 1331564676})
+        self.assertTrue(event in self.command.action_queue.event_queue.events)
+
+    def test_retryable_failure_nothing_on_other_errors(self):
+        """Test nothing is pushed on other errors."""
+        failure = Failure(twisted_error.ConnectionLost())
+        self.command.handle_retryable(failure)
+        event_names = [x[0]
+                       for x in self.command.action_queue.event_queue.events]
+        self.assertFalse('SYS_QUOTA_EXCEEDED' in event_names)
 
     def test_AQ_UPLOAD_FILE_PROGRESS_is_valid_event(self):
         """AQ_UPLOAD_FILE_PROGRESS is a valid event."""
@@ -4871,9 +4887,10 @@
         """Retry the command immediately."""
         finished = defer.Deferred()
         called = []
-        failure = Failure(twisted_error.ConnectionDone()) # retryable!
-        run_deferreds = [defer.fail(failure), defer.succeed('finish')]
+        exc = twisted_error.ConnectionDone() # retryable!
+        run_deferreds = [defer.fail(Failure(exc)), defer.succeed('finish')]
         self.cmd._run = lambda: called.append('run') or run_deferreds.pop(0)
+        self.cmd.handle_retryable = lambda f: called.append(f.value)
 
         # check handle success (failure is never called because it's retried)
         self.cmd.handle_success = lambda a: called.append(a)
@@ -4892,7 +4909,7 @@
         yield finished
 
         # all check
-        self.assertEqual(called, ['run', 'run', 'finish'])
+        self.assertEqual(called, ['run', exc, 'run', 'finish'])
         self._check_finished_ok()
 
     def test_retry_conditions_solved(self):
@@ -4985,6 +5002,23 @@
         self.assertEqual(called, [1])
         self._check_finished_ok()
 
+    def test_cancel_while_waiting_conditions(self):
+        """Cancel the command while waiting for conditions."""
+        # make it not runnable, and fake the pathlock to test releasing
+        self.cmd.is_runnable = False
+        released = []
+        self.cmd._acquire_pathlock = lambda: defer.succeed(
+                                                lambda: released.append(True))
+
+        # let the command go (will stuck because not runnable), and
+        # cancel in the middle
+        self.cmd.go()
+        self.cmd.cancel()
+
+        # all check
+        self._check_finished_ok()
+        self.assertTrue(released)
+
     def test_marker_error_while_pathclocked(self):
         """The marker errbacks while the command is waiting the pathlock."""
         # monkeypatch methods to flag called and test "while running"

=== modified file 'ubuntuone/syncdaemon/action_queue.py'
--- ubuntuone/syncdaemon/action_queue.py	2011-03-02 14:24:13 +0000
+++ ubuntuone/syncdaemon/action_queue.py	2011-03-02 19:30:59 +0000
@@ -1201,12 +1201,14 @@
     def resume(self):
         """Unlock the command because the queue is back alive."""
         if self.wait_for_queue is not None:
+            self.log.debug('resuming')
             self.wait_for_queue.callback(True)
             self.wait_for_queue = None
 
     def check_conditions(self):
         """If conditions are ok, run the command again."""
         if self.is_runnable and self.wait_for_conditions is not None:
+            self.log.debug('unblocking conditions')
             self.wait_for_conditions.callback(True)
             self.wait_for_conditions = None
 
@@ -1295,6 +1297,7 @@
 
                 if exc.__class__ in self.retryable_errors:
                     self.log.debug('retrying')
+                    self.handle_retryable(Failure(exc))
                     continue
                 else:
                     self.handle_failure(Failure(exc))
@@ -1312,13 +1315,21 @@
     def cancel(self):
         """Cancel the command.
 
+        It also triggers the wait_for_condition deferred (but not the
+        wait_for_queue one, as if the queue is not active, it's pointless
+        to release the pathlock for other commands).
+
         Do nothing if already cancelled (as cancellation can come from other
         thread, it can come at any time, so we need to support double
         cancellation safely).
         """
         if self.cancelled:
             return
+
         self.cancelled = True
+        if self.wait_for_conditions is not None:
+            self.wait_for_conditions.callback(True)
+            self.wait_for_conditions = None
         self.cleanup()
         self.finish()
 
@@ -1332,6 +1343,9 @@
     def handle_failure(self, failure):
         """Do anthing that's needed to handle failure of the operation."""
 
+    def handle_retryable(self, failure):
+        """Had that failure, but the command will be retried."""
+
     def __str__(self, str_attrs=None):
         """Return a str representation of the instance."""
         if str_attrs is None:
@@ -2416,17 +2430,19 @@
                  new_generation=request.new_generation)
         self.action_queue.event_queue.push('AQ_UPLOAD_FINISHED', **d)
 
-    def handle_failure(self, failure):
-        """It didn't work! Push the event."""
-        if getattr(self.tempfile, 'name', None) is not None:
-            os.unlink(self.tempfile.name)
-
+    def handle_retryable(self, failure):
+        """For a retryable failure."""
         if failure.check(protocol_errors.QuotaExceededError):
             error = failure.value
             self.action_queue.event_queue.push('SYS_QUOTA_EXCEEDED',
                                                volume_id=str(error.share_id),
                                                free_bytes=error.free_bytes)
 
+    def handle_failure(self, failure):
+        """It didn't work! Push the event."""
+        if getattr(self.tempfile, 'name', None) is not None:
+            os.unlink(self.tempfile.name)
+
         self.action_queue.event_queue.push('AQ_UPLOAD_ERROR',
                                            error=failure.getErrorMessage(),
                                            share_id=self.share_id,