From 80055b7ec0aced1ee773791ab42e3957808ae480 Mon Sep 17 00:00:00 2001 From: Ken Roberts Date: Tue, 17 Feb 2015 13:27:51 -0800 Subject: [PATCH 1/5] Fix ftw socket.timeout bug 1422683 --- openlp/core/ui/firsttimeform.py | 25 +++++++++++++++---- .../openlp_core_ui/test_firsttimeform.py | 24 ++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/openlp/core/ui/firsttimeform.py b/openlp/core/ui/firsttimeform.py index 98e2bf78f..c61c52993 100644 --- 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 @@ -403,8 +404,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 +423,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) @@ -617,6 +618,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 +628,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 +639,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 +650,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 += '{}
'.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:
{}'.format(file_list))) + msg.setStandardButtons(msg.Ok) + ans = msg.exec_() return True def _set_plugin_status(self, field, tag): diff --git a/tests/functional/openlp_core_ui/test_firsttimeform.py b/tests/functional/openlp_core_ui/test_firsttimeform.py index 4be8ec063..54c1a7b1d 100644 --- 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') From 2b63b5d3adde7358d9ee67dbff48f87384e5ff3f Mon Sep 17 00:00:00 2001 From: Ken Roberts Date: Wed, 18 Feb 2015 08:13:36 -0800 Subject: [PATCH 2/5] Add extra exceptions to get_web_page --- openlp/core/utils/__init__.py | 38 +++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/openlp/core/utils/__init__.py b/openlp/core/utils/__init__.py index 44399eb60..bbf9e026b 100644 --- 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,33 @@ 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): - 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 + 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 + except socket.timeout: + log.exception('Socket timeout: {}'.format(url)) + page = None + except ConnectionRefusedError: + log.exception('ConnectionRefused: {}'.format(url)) + page = None + break + except ConnectionError: + log.exception('Connection error: {}'.format(url)) + page = None 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 From 33d8dcfa508da7274f956a33c6f37a80b3539328 Mon Sep 17 00:00:00 2001 From: Ken Roberts Date: Wed, 18 Feb 2015 12:40:39 -0800 Subject: [PATCH 3/5] Reraise exception outside of checks --- openlp/core/utils/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/openlp/core/utils/__init__.py b/openlp/core/utils/__init__.py index bbf9e026b..d4c4ca46f 100644 --- a/openlp/core/utils/__init__.py +++ b/openlp/core/utils/__init__.py @@ -417,6 +417,9 @@ def get_web_page(url, header=None, update_openlp=False): except ConnectionError: log.exception('Connection error: {}'.format(url)) page = None + except: + # Don't know what's happening, so reraise the original + raise if update_openlp: Registry().get('application').process_events() if not page: From fd81d2d80ab5366b050cdf4fa46ca1ee18403269 Mon Sep 17 00:00:00 2001 From: Ken Roberts Date: Thu, 19 Feb 2015 07:34:09 -0800 Subject: [PATCH 4/5] Fix thumbnail download blocking events --- openlp/core/ui/firsttimeform.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/openlp/core/ui/firsttimeform.py b/openlp/core/ui/firsttimeform.py index c61c52993..22218f983 100644 --- a/openlp/core/ui/firsttimeform.py +++ b/openlp/core/ui/firsttimeform.py @@ -62,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): @@ -251,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='') @@ -265,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): """ @@ -448,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): """ From b62bb37776faa12139edcc051f84a591e205f35e Mon Sep 17 00:00:00 2001 From: Ken Roberts Date: Thu, 19 Feb 2015 09:17:04 -0800 Subject: [PATCH 5/5] Reraise exception if retries > CONNECTION_RETRIES --- openlp/core/utils/__init__.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/openlp/core/utils/__init__.py b/openlp/core/utils/__init__.py index d4c4ca46f..ea896fad7 100644 --- a/openlp/core/utils/__init__.py +++ b/openlp/core/utils/__init__.py @@ -407,16 +407,24 @@ def get_web_page(url, header=None, update_openlp=False): log.exception('URLError on {}'.format(url)) log.exception('URLError: {}'.format(err.reason)) page = None + if retries > CONNECTION_RETRIES: + raise 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