From a1083886df0aac8d86382e6081c2e4db19d2c81a Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 19 Apr 2013 14:12:11 -0400 Subject: [PATCH] Some improvements and bug fixes. 1. Discard body even if response code indicates a problem, when doing cloud backend HTTP requests. I believe this was triggering a bug in Twistd. 2. Google backend retries on 403 and other 4xx codes, not just 401. 3. More logging. --- .../storage/backends/cloud/cloud_common.py | 3 +++ .../cloud/googlestorage/googlestorage_container.py | 13 +++++++++++-- src/allmydata/test/test_storage.py | 10 ++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage/backends/cloud/cloud_common.py b/src/allmydata/storage/backends/cloud/cloud_common.py index 72ac66e6..a62d94f3 100644 --- a/src/allmydata/storage/backends/cloud/cloud_common.py +++ b/src/allmydata/storage/backends/cloud/cloud_common.py @@ -401,12 +401,14 @@ class ContainerRetryMixin: retry = False if retry: + log.msg("Rescheduling failed task for retry in %d seconds." % (BACKOFF_SECONDS_BEFORE_RETRY[trynum-1],)) d = task.deferLater(self._reactor, BACKOFF_SECONDS_BEFORE_RETRY[trynum-1], operation, *args, **kwargs) d.addErrback(self._handle_error, trynum+1, first_err_and_tb, description, operation, *args, **kwargs) return d # If we get an error response for which _react_to_error says we should not retry, # raise that error even if the request was itself a retry. + log.msg("Giving up, no retry for %s" % (err,)) raise err.__class__, err, tb @@ -673,6 +675,7 @@ class HTTPClientMixin: what=what, code=response.code, phrase=response.phrase, level=log.OPERATIONAL) if response.code < 200 or response.code >= 300: + response.deliverBody(Discard()) raise self.ServiceError(None, response.code, message="unexpected response code %r %s" % (response.code, response.phrase)) diff --git a/src/allmydata/storage/backends/cloud/googlestorage/googlestorage_container.py b/src/allmydata/storage/backends/cloud/googlestorage/googlestorage_container.py index 369c9114..47c2ff24 100644 --- a/src/allmydata/storage/backends/cloud/googlestorage/googlestorage_container.py +++ b/src/allmydata/storage/backends/cloud/googlestorage/googlestorage_container.py @@ -29,6 +29,7 @@ except ImportError: from zope.interface import implements +from allmydata.util import log from allmydata.storage.backends.cloud.cloud_common import IContainer, \ ContainerItem, ContainerListing, CommonContainerMixin @@ -70,7 +71,13 @@ class AuthenticationClient(object): # Generally using a task-specific thread pool is better than using # the reactor one. However, this particular call will only run # once an hour, so it's not likely to tie up all the threads. - return self._deferToThread(self._credentials.refresh, httplib2.Http()) + log.msg("Reauthenticating against Google Cloud Storage.") + def finished(result): + log.msg("Done reauthenticating against Google Cloud Storage.") + return result + d = self._deferToThread(self._credentials.refresh, httplib2.Http()) + d.addBoth(finished) + return d return self._lock.run(run) def get_authorization_header(self): @@ -108,7 +115,9 @@ class GoogleStorageContainer(CommonContainerMixin): self._project_id = project_id # Only need for bucket creation/deletion def _react_to_error(self, response_code): - if response_code == UNAUTHORIZED: + if response_code >= 400 and response_code < 500: + # Unauthorized/forbidden/etc. we should retry, eventually we will + # reauthenticate: return True else: return CommonContainerMixin._react_to_error(self, response_code) diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 01a92651..8e3843bb 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -1118,6 +1118,16 @@ class GoogleStorageBackend(unittest.TestCase, CloudStorageBackendMixin): third.callback(done) self.assertEqual(result, [done]) + def test_react_to_error(self): + """ + GoogleStorageContainer._react_to_error() will return True (i.e. retry) + for any response code between 400 and 599. + """ + self.assertFalse(self.container._react_to_error(399)) + self.assertFalse(self.container._react_to_error(600)) + for i in range(400, 600): + self.assertTrue(self.container._react_to_error(i)) + def test_head_object(self): """ GoogleStorageContainer.head_object() sends the appropriate HTTP -- 2.45.2