Merge lp:~nataliabidart/ubuntuone-client/run-upload-run into lp:ubuntuone-client

[Natalia Bidart]

- Make the Upload process do not close the tempfile until is finished (LP: #872924).

[Facundo Batista (facundo)]

Thanks!

review: Approve

[lp:~nataliabidart/ubuntuone-client/run-upload-run]

1156. By Natalia Bidart on 2011-10-28

Merged trunk in.

[Roman Yepishev (rye)]

Looks good. No errors while disconnecting/reconnecting the network or SD.

review: Approve (fieldtest)

=== modified file 'tests/syncdaemon/test_action_queue.py'
--- tests/syncdaemon/test_action_queue.py	2011-10-27 11:39:43 +0000
+++ tests/syncdaemon/test_action_queue.py	2011-10-28 19:48:24 +0000
@@ -106,9 +106,12 @@
 
 class FakeTempFile(object):
     """Fake temporary file."""
+
     def __init__(self, tmpdir):
+        self.closed = 0  # be able to count how may close calls we had
         self.name = os.path.join(tmpdir, 'remove-me.zip')
         open_file(self.name, 'w').close()
+        self.close = lambda: setattr(self, 'closed', self.closed + 1)
 
 
 class FakeCommand(object):
@@ -3349,16 +3352,29 @@
         events = [('AQ_UPLOAD_ERROR', kwargs)]
         self.assertEqual(events, self.command.action_queue.event_queue.events)
 
-    def test_handle_failure_removes_temp_file(self):
-        """Test temp file is removed on failure."""
+    def test_finish_closes_temp_file(self):
+        """Test temp file is closed when the command finishes."""
+        self.command.tempfile = FakeTempFile(self.tmpdir)
+        assert self.command.tempfile.closed == 0
+
+        self.command.finish()
+        self.assertEqual(1, self.command.tempfile.closed)
+
+    def test_finish_removes_temp_file(self):
+        """Test temp file is removed when the command finishes."""
         self.command.tempfile = FakeTempFile(self.tmpdir)
         assert path_exists(self.command.tempfile.name)
 
-        msg = 'Something went wrong'
-        failure = Failure(DefaultException(msg))
-        self.command.handle_failure(failure=failure)
+        self.command.finish()
         self.assertFalse(path_exists(self.command.tempfile.name))
 
+    def test_finish_handles_temp_file_none(self):
+        """Test temp file can be None when calling finish."""
+        self.command.tempfile = None
+
+        self.command.finish()
+        self.assertEqual(self.command.tempfile, None)  # nothing changed
+
     def test_retryable_failure_push_quota_exceeded_if_that_error(self):
         """Test SYS_QUOTA_EXCEEDED is pushed on QuotaExceededError."""
         protocol_msg = protocol_pb2.Message()

=== modified file 'ubuntuone/syncdaemon/action_queue.py'
--- ubuntuone/syncdaemon/action_queue.py	2011-10-14 20:02:23 +0000
+++ ubuntuone/syncdaemon/action_queue.py	2011-10-28 19:48:24 +0000
@@ -37,7 +37,7 @@
 import zlib
 
 from collections import deque, defaultdict
-from functools import wraps, partial
+from functools import partial
 from urllib import urlencode
 from urllib2 import urlopen, Request, HTTPError
 from urlparse import urljoin
@@ -77,18 +77,6 @@
 TRANSFER_PROGRESS_THRESHOLD = 64 * 1024 * 1024
 
 
-def passit(func):
-    """Pass the value on for the next deferred, while calling func with it."""
-
-    @wraps(func)
-    def wrapper(a):
-        """Do it."""
-        func(a)
-        return a
-
-    return wrapper
-
-
 class DeferredInterrupted(Exception):
     """To stop the run when pausing."""
 
@@ -2497,6 +2485,11 @@
 
     def finish(self):
         """Release the semaphore if already acquired."""
+        if self.tempfile is not None:
+            # clean the temporary file
+            self.tempfile.close()
+            remove_file(self.tempfile.name)
+
         if self.tx_semaphore is not None:
             self.tx_semaphore = self.tx_semaphore.release()
             self.log.debug('semaphore released')
@@ -2520,9 +2513,7 @@
             upload_id=self.upload_id, upload_id_cb=self._upload_id_cb,
             magic_hash=magic_hash)
         self.upload_req = req
-        d = req.deferred
-        d.addBoth(passit(lambda _: self.tempfile.close()))
-        return d
+        return req.deferred
 
     def _upload_id_cb(self, upload_id):
         """Handle the received upload_id, save it in the metadata."""
@@ -2544,9 +2535,6 @@
 
     def handle_success(self, request):
         """It worked! Push the event."""
-        # remove the temporary file
-        remove_file(self.tempfile.name)
-
         # send the event
         d = dict(share_id=self.share_id, node_id=self.node_id, hash=self.hash,
                  new_generation=request.new_generation)
@@ -2562,7 +2550,6 @@
 
     def handle_failure(self, failure):
         """It didn't work! Push the event."""
-        remove_file(self.tempfile.name)
         self.action_queue.event_queue.push('AQ_UPLOAD_ERROR',
                                            error=failure.getErrorMessage(),
                                            share_id=self.share_id,