From 72956174187fa2fccbb060d04d4809797657e029 Mon Sep 17 00:00:00 2001 From: Victor Shyba Date: Fri, 27 Oct 2017 19:39:25 -0300 Subject: [bug] there is no retry limit for usual transfers Retry limit was originally specified in #8825 as a protection mechanism, but #8822 (retry) doesn't specify a retry limit. In fact, blobs is supposed to retry until transfer is complete using timed delays between attempts, but never giving up. -- Related: #8822 -- Related: #8825 --- src/leap/soledad/client/_db/blobs/__init__.py | 21 +++++++------ src/leap/soledad/client/_db/blobs/sync.py | 16 ++-------- tests/blobs/test_blob_manager.py | 45 --------------------------- 3 files changed, 14 insertions(+), 68 deletions(-) diff --git a/src/leap/soledad/client/_db/blobs/__init__.py b/src/leap/soledad/client/_db/blobs/__init__.py index 3c8facba..2df315dc 100644 --- a/src/leap/soledad/client/_db/blobs/__init__.py +++ b/src/leap/soledad/client/_db/blobs/__init__.py @@ -107,6 +107,9 @@ class BlobManager(BlobsSynchronizer): The BlobManager can list, put, get, set flags and synchronize blobs stored in local and remote storages. """ + max_decrypt_retries = 3 + concurrent_transfers_limit = 3 + concurrent_writes_limit = 100 def __init__( self, local_path, remote, key, secret, user, token=None, @@ -128,9 +131,6 @@ class BlobManager(BlobsSynchronizer): :type cert_file: str """ super(BlobsSynchronizer, self).__init__() - self.max_retries = 3 - self.concurrent_transfers_limit = 3 - self.concurrent_writes_limit = 100 if local_path: mkdir_p(os.path.dirname(local_path)) self.local = SQLiteBlobBackend(local_path, key=key, user=user) @@ -307,22 +307,23 @@ class BlobManager(BlobsSynchronizer): _, retries = yield self.local.get_sync_status(blob_id) if isinstance(e, InvalidBlob): + max_retries = self.max_decrypt_retries message = "Corrupted blob received from server! ID: %s\n" message += "Error: %r\n" message += "Retries: %s - Attempts left: %s\n" message += "This is either a bug or the contents of the " message += "blob have been tampered with. Please, report to " message += "your provider's sysadmin and submit a bug report." - message %= (blob_id, e, retries, (self.max_retries - retries)) + message %= (blob_id, e, retries, (max_retries - retries)) logger.error(message) - yield self.local.increment_retries(blob_id) + yield self.local.increment_retries(blob_id) - if (retries + 1) >= self.max_retries: - failed_download = SyncStatus.FAILED_DOWNLOAD - yield self.local.update_sync_status( - blob_id, failed_download, namespace=namespace) - raise MaximumRetriesError(e) + if (retries + 1) >= max_retries: + failed_download = SyncStatus.FAILED_DOWNLOAD + yield self.local.update_sync_status( + blob_id, failed_download, namespace=namespace) + raise MaximumRetriesError(e) raise RetriableTransferError(e) diff --git a/src/leap/soledad/client/_db/blobs/sync.py b/src/leap/soledad/client/_db/blobs/sync.py index a1b5cc4c..ee10443d 100644 --- a/src/leap/soledad/client/_db/blobs/sync.py +++ b/src/leap/soledad/client/_db/blobs/sync.py @@ -22,7 +22,7 @@ from twisted.internet import reactor from twisted.logger import Logger from twisted.internet import error from .sql import SyncStatus -from .errors import MaximumRetriesError, RetriableTransferError +from .errors import RetriableTransferError logger = Logger() @@ -90,18 +90,8 @@ class BlobsSynchronizer(object): logger.info("Sending blob to server (%d/%d): %s" % (i, total, blob_id)) fd = yield self.local.get(blob_id, namespace=namespace) - try: - yield self._encrypt_and_upload(blob_id, fd) - yield self.local.update_sync_status(blob_id, SyncStatus.SYNCED) - except Exception as e: - yield self.local.increment_retries(blob_id) - res = yield self.local.get_sync_status(blob_id) - _, retries = res - if (retries + 1) > self.max_retries: - failed_upload = SyncStatus.FAILED_UPLOAD - yield self.local.update_sync_status(blob_id, failed_upload) - raise MaximumRetriesError(e) - raise e + yield self._encrypt_and_upload(blob_id, fd) + yield self.local.update_sync_status(blob_id, SyncStatus.SYNCED) @defer.inlineCallbacks def fetch_missing(self, namespace=''): diff --git a/tests/blobs/test_blob_manager.py b/tests/blobs/test_blob_manager.py index 4b2b1135..f1872ab1 100644 --- a/tests/blobs/test_blob_manager.py +++ b/tests/blobs/test_blob_manager.py @@ -23,7 +23,6 @@ from leap.soledad.client._document import BlobDoc from leap.soledad.client._db.blobs import BlobManager, FIXED_REV from leap.soledad.client._db.blobs import BlobAlreadyExistsError from leap.soledad.client._db.blobs import SyncStatus -from leap.soledad.client._db.blobs import RetriableTransferError from io import BytesIO from mock import Mock from uuid import uuid4 @@ -167,50 +166,6 @@ class BlobManagerTestCase(unittest.TestCase): local_list = yield self.manager.local_list_status(pending_upload) self.assertIn(blob_id, local_list) - @defer.inlineCallbacks - @pytest.mark.usefixtures("method_tmpdir") - def test_upload_retry_limit(self): - # prepare the manager to fail accordingly - self.manager.remote_list = Mock(return_value=[]) - self.manager._encrypt_and_upload = Mock( - side_effect=RetriableTransferError) - # put a blob in local storage - content, blob_id = "Blob content", uuid4().hex - yield self.manager.local.put(blob_id, BytesIO(content), len(content)) - pending = SyncStatus.PENDING_UPLOAD - yield self.manager.local.update_sync_status(blob_id, pending) - # try to send missing - with pytest.raises(defer.FirstError): - yield self.manager.send_missing() - # assert failed state and number of retries - failed_upload = SyncStatus.FAILED_UPLOAD - local_list = yield self.manager.local_list_status(failed_upload) - self.assertIn(blob_id, local_list) - sync_status, retries = \ - yield self.manager.local.get_sync_status(blob_id) - self.assertEqual(failed_upload, sync_status) - self.assertEqual(self.manager.max_retries, retries) - - @defer.inlineCallbacks - @pytest.mark.usefixtures("method_tmpdir") - def test_download_retry_limit(self): - # prepare the manager to fail accordingly - blob_id = uuid4().hex - self.manager.local_list_status = Mock(return_value=[blob_id]) - self.manager._download_and_decrypt = Mock( - side_effect=RetriableTransferError) - # try to fetch missing - with pytest.raises(defer.FirstError): - yield self.manager.fetch_missing() - # assert failed state and number of retries - failed_download = SyncStatus.FAILED_DOWNLOAD - local_list = yield self.manager.local.list_status(failed_download) - self.assertIn(blob_id, local_list) - sync_status, retries = \ - yield self.manager.local.get_sync_status(blob_id) - self.assertEqual(failed_download, sync_status) - self.assertEqual(self.manager.max_retries, retries) - @defer.inlineCallbacks @pytest.mark.usefixtures("method_tmpdir") def test_local_list_doesnt_include_unavailable_blobs(self): -- cgit v1.2.3