Fix bug 1422683: Added exception checks to get_web_page() to help with changed description. Rearrange calls so thumbnail downloads don't hang other event threads. Reraise exception when retries > CONNECTION_RETRIES.

bzr-revno: 2512 Fixes: https://launchpad.net/bugs/1422683
2015-02-21 21:11:25 +02:00 · 2015-02-21 21:11:25 +02:00 · 85b8cb2c50
parent 23ef01bce2 b62bb37776
commit 85b8cb2c50
3 changed files with 80 additions and 21 deletions
--- a/openlp/core/ui/firsttimeform.py
+++ b/openlp/core/ui/firsttimeform.py
@ -25,6 +25,7 @@ This module contains the first time wizard.
 import hashlib
 import logging
 import os
+import socket
 import time
 import urllib.request
 import urllib.parse
@ -61,6 +62,7 @@ class ThemeScreenshotWorker(QtCore.QObject):
        self.filename = filename
        self.sha256 = sha256
        self.screenshot = screenshot
+        socket.setdefaulttimeout(CONNECTION_TIMEOUT)
        super(ThemeScreenshotWorker, self).__init__()

    def run(self):
@ -250,7 +252,6 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties):
            # Download the theme screenshots
            themes = self.config.get('themes', 'files').split(',')
            for theme in themes:
-                self.application.process_events()
                title = self.config.get('theme_%s' % theme, 'title')
                filename = self.config.get('theme_%s' % theme, 'filename')
                sha256 = self.config.get('theme_%s' % theme, 'sha256', fallback='')
@ -264,6 +265,7 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties):
                worker.finished.connect(thread.quit)
                worker.moveToThread(thread)
                thread.start()
+            self.application.process_events()

    def set_defaults(self):
        """
@ -403,8 +405,8 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties):
        retries = 0
        while True:
            try:
-                url_file = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT)
                filename = open(f_path, "wb")
+                url_file = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT)
                if sha256:
                    hasher = hashlib.sha256()
                # Download until finished or canceled.
@ -422,7 +424,7 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties):
                    log.error('sha256 sums did not match for file: {}'.format(f_path))
                    os.remove(f_path)
                    return False
-            except urllib.error.URLError:
+            except (urllib.error.URLError, socket.timeout) as err:
                trace_error_handler(log)
                filename.close()
                os.remove(f_path)
@ -447,8 +449,8 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties):
        for index, theme in enumerate(themes):
            screenshot = self.config.get('theme_%s' % theme, 'screenshot')
            item = self.themes_list_widget.item(index)
-            # if item:
-            item.setIcon(build_icon(os.path.join(gettempdir(), 'openlp', screenshot)))
+            if item:
+                item.setIcon(build_icon(os.path.join(gettempdir(), 'openlp', screenshot)))

    def _get_file_size(self, url):
        """
@ -617,6 +619,7 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties):
        songs_destination = os.path.join(gettempdir(), 'openlp')
        bibles_destination = AppLocation.get_section_data_path('bibles')
        themes_destination = AppLocation.get_section_data_path('themes')
+        missed_files = []
        # Download songs
        for i in range(self.songs_list_widget.count()):
            item = self.songs_list_widget.item(i)
@ -626,7 +629,7 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties):
                self.previous_size = 0
                destination = os.path.join(songs_destination, str(filename))
                if not self.url_get_file('%s%s' % (self.songs_url, filename), destination, sha256):
-                    return False
+                    missed_files.append('Song: {}'.format(filename))
        # Download Bibles
        bibles_iterator = QtGui.QTreeWidgetItemIterator(self.bibles_tree_widget)
        while bibles_iterator.value():
@ -637,7 +640,7 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties):
                self.previous_size = 0
                if not self.url_get_file('%s%s' % (self.bibles_url, bible), os.path.join(bibles_destination, bible),
                                         sha256):
-                    return False
+                    missed_files.append('Bible: {}'.format(bible))
            bibles_iterator += 1
        # Download themes
        for i in range(self.themes_list_widget.count()):
@ -648,7 +651,20 @@ class FirstTimeForm(QtGui.QWizard, UiFirstTimeWizard, RegistryProperties):
                self.previous_size = 0
                if not self.url_get_file('%s%s' % (self.themes_url, theme), os.path.join(themes_destination, theme),
                                         sha256):
-                    return False
+                    missed_files.append('Theme: {}'.format(theme))
+        if missed_files:
+            file_list = ''
+            for entry in missed_files:
+                file_list += '{}<br \>'.format(entry)
+            msg = QtGui.QMessageBox()
+            msg.setIcon(QtGui.QMessageBox.Warning)
+            msg.setWindowTitle(translate('OpenLP.FirstTimeWizard', 'Network Error'))
+            msg.setText(translate('OpenLP.FirstTimeWizard', 'Unable to download some files'))
+            msg.setInformativeText(translate('OpenLP.FirstTimeWizard',
+                                             'The following files were not able to be '
+                                             'downloaded:<br \>{}'.format(file_list)))
+            msg.setStandardButtons(msg.Ok)
+            ans = msg.exec_()
        return True

    def _set_plugin_status(self, field, tag):
--- a/openlp/core/utils/init.py
+++ b/openlp/core/utils/init.py
@ -29,6 +29,7 @@ import locale
 import os
 import platform
 import re
+import socket
 import time
 from shutil import which
 from subprocess import Popen, PIPE
@ -394,26 +395,44 @@ def get_web_page(url, header=None, update_openlp=False):
        req.add_header('User-Agent', user_agent)
    if header:
        req.add_header(header[0], header[1])
-    page = None
    log.debug('Downloading URL = %s' % url)
-    retries = 1
-    while True:
+    retries = 0
+    while retries <= CONNECTION_RETRIES:
+        retries += 1
+        time.sleep(0.1)
        try:
            page = urllib.request.urlopen(req, timeout=CONNECTION_TIMEOUT)
-            log.debug('Downloaded URL = %s' % page.geturl())
-        except (urllib.error.URLError, ConnectionError):
+            log.debug('Downloaded page {}'.format(page.geturl()))
+        except urllib.error.URLError as err:
+            log.exception('URLError on {}'.format(url))
+            log.exception('URLError: {}'.format(err.reason))
+            page = None
            if retries > CONNECTION_RETRIES:
-                log.exception('The web page could not be downloaded')
                raise
-            else:
-                retries += 1
-                time.sleep(0.1)
-                continue
-        break
-    if not page:
-        return None
+        except socket.timeout:
+            log.exception('Socket timeout: {}'.format(url))
+            page = None
+            if retries > CONNECTION_RETRIES:
+                raise
+        except ConnectionRefusedError:
+            log.exception('ConnectionRefused: {}'.format(url))
+            page = None
+            if retries > CONNECTION_RETRIES:
+                raise
+            break
+        except ConnectionError:
+            log.exception('Connection error: {}'.format(url))
+            page = None
+            if retries > CONNECTION_RETRIES:
+                raise
+        except:
+            # Don't know what's happening, so reraise the original
+            raise
    if update_openlp:
        Registry().get('application').process_events()
+    if not page:
+        log.exception('{} could not be downloaded'.format(url))
+        return None
    log.debug(page)
    return page

--- a/tests/functional/openlp_core_ui/test_firsttimeform.py
+++ b/tests/functional/openlp_core_ui/test_firsttimeform.py
@ -23,6 +23,8 @@
 Package to test the openlp.core.ui.firsttimeform package.
 """
 import os
+import socket
+import tempfile
 import urllib
 from unittest import TestCase

@ -70,6 +72,11 @@ class TestFirstTimeForm(TestCase, TestMixin):
        self.app.process_events = lambda: None
        Registry.create()
        Registry().register('application', self.app)
+        self.tempfile = os.path.join(tempfile.gettempdir(), 'testfile')
+
+    def tearDown(self):
+        if os.path.isfile(self.tempfile):
+            os.remove(self.tempfile)

    def initialise_test(self):
        """
@ -229,3 +236,20 @@ class TestFirstTimeForm(TestCase, TestMixin):
        # THEN: the critical_error_message_box should have been called
        self.assertEquals(mocked_message_box.mock_calls[1][1][0], 'Network Error 407',
                          'first_time_form should have caught Network Error')
+
+    @patch('openlp.core.ui.firsttimeform.urllib.request.urlopen')
+    def socket_timeout_test(self, mocked_urlopen):
+        """
+        Test socket timeout gets caught
+        """
+        # GIVEN: Mocked urlopen to fake a network disconnect in the middle of a download
+        first_time_form = FirstTimeForm(None)
+        first_time_form.initialize(MagicMock())
+        mocked_urlopen.side_effect = socket.timeout()
+
+        # WHEN: Attempt to retrieve a file
+        first_time_form.url_get_file(url='http://localhost/test', f_path=self.tempfile)
+
+        # THEN: socket.timeout should have been caught
+        # NOTE: Test is if $tmpdir/tempfile is still there, then test fails since ftw deletes bad downloaded files
+        self.assertFalse(os.path.exists(self.tempfile), 'FTW url_get_file should have caught socket.timeout')