Merge lp:~mvo/software-center/downloader-fix-race839462-again into lp:software-center

[Michael Vogt]

This branch fixes a race condition in the SimpleFileDownloader when the
file is downloaded but the signal did not get delivered yet by the gtk
event loop.

[Dave Morley (davmor2)]

Tested the code on my box that originally displayed the issue and everything seems to be working fine.

review: Approve (tested the code)

[Łukasz Czyżykowski (lukasz-czyzykowski)]

review: Approve

[Gary Lasker (gary-lasker)]

review: Approve

=== modified file 'softwarecenter/utils.py'
--- softwarecenter/utils.py	2012-09-17 10:33:16 +0000
+++ softwarecenter/utils.py	2012-09-24 08:00:27 +0000
@@ -921,11 +921,28 @@
         f.query_info_async(Gio.FILE_ATTRIBUTE_STANDARD_SIZE, 0, 0,
                            self._cancellable,
                            self._check_url_reachable_and_then_download_cb,
-                           None)
-
-    def _check_url_reachable_and_then_download_cb(self, f, result,
-                                                  user_data=None):
+                           url)
+
+    def _ensure_correct_url(self, want_url):
+        """This function will ensure that the url we requested to download
+           earlier matches that is now downloaded.
+        """
+        # this function is needed as there is a rance condition when the
+        # operation is finished but the signal is not delivered yet (its
+        # still in the gtk event loop). in this case there is nothing to
+        # self._cancel but self.url/self.dest_file_path will still point to
+        # the wrong file
+        if self.url != want_url:
+            self.LOG.warn("url changed from '%s' to '%s'" % (
+                    want_url, self.url))
+            return False
+        return True
+
+    def _check_url_reachable_and_then_download_cb(self, f, result, want_url):
         self.LOG.debug("_check_url_reachable_and_then_download_cb: %s" % f)
+        if not self._ensure_correct_url(want_url):
+            return
+        # normal operation
         try:
             info = f.query_info_finish(result)
             etag = info.get_etag()
@@ -935,15 +952,17 @@
                                                         etag))
             # url is reachable, now download the file
             f.load_contents_async(
-                self._cancellable, self._file_download_complete_cb, None)
+                self._cancellable, self._file_download_complete_cb, want_url)
         except GObject.GError as e:
             self.LOG.debug("file *not* reachable %s" % self.url)
             self.emit('file-url-reachable', False)
             self.emit('error', GObject.GError, e)
         del f
 
-    def _file_download_complete_cb(self, f, result, path=None):
+    def _file_download_complete_cb(self, f, result, want_url):
         self.LOG.debug("file download completed %s" % self.dest_file_path)
+        if not self._ensure_correct_url(want_url):
+            return
         # The result from the download is actually a tuple with three
         # elements (content, size, etag?)
         # The first element is the actual content so let's grab that

=== modified file 'tests/test_downloader.py'
--- tests/test_downloader.py	2012-08-15 12:11:04 +0000
+++ tests/test_downloader.py	2012-09-24 08:00:27 +0000
@@ -1,7 +1,10 @@
+import glob
 import os
+import tempfile
 import unittest
 
 from tests.utils import (
+    do_events,
     do_events_with_sleep,
     setup_test_env,
 )
@@ -60,5 +63,28 @@
         self.assertTrue(self._image_is_reachable)
         self.assertTrue(os.path.exists(self.DOWNLOAD_FILENAME))
 
+    @unittest.skip("highly dependant on network speed so disabled by default")
+    def test_download_race_lp839462(self):
+        hosts = ["google", "ubuntu"]
+        tmpdir = tempfile.mkdtemp()
+        # mvo: in order to reproduce the race these paramters can be tweaked
+        # its highly depedant on network latency, for me it hit 1/3 times
+        # at around i=7,8 - if the race ever hits again this will be a useful
+        # starting point
+        for i in range(5,15):
+            for host in hosts:
+                url = "http://www.%s.com/" % host
+                target = os.path.join(tmpdir, host)
+                self.downloader.download_file(url, target)
+                do_events_with_sleep(iterations=i)
+            for host in hosts:
+                downloaded_file = os.path.join(tmpdir, host)
+                if os.path.exists(downloaded_file):
+                    self.assertTrue(
+                        host in open(downloaded_file).read().lower(),
+                        "file '%s' contains wrong content" % downloaded_file)
+            for f in glob.glob(tmpdir+"/*"):
+                os.unlink(f)
+
 if __name__ == "__main__":
     unittest.main()