Merge "internal_client: Don't retry when we expect the same reponse"

3 files changed, 104 insertions, 74 deletions

diff --git a/swift/common/internal_client.py b/swift/common/internal_client.py
index dbfce3051..a5a9ad340 100644
--- a/swift/common/internal_client.py
+++ b/swift/common/internal_client.py
@@ -26,9 +26,11 @@ from time import gmtime, strftime, time
 from zlib import compressobj
 
 from swift.common.exceptions import ClientException
-from swift.common.http import HTTP_NOT_FOUND, HTTP_MULTIPLE_CHOICES
+from swift.common.http import HTTP_NOT_FOUND, HTTP_MULTIPLE_CHOICES, \
+    HTTP_CONFLICT
 from swift.common.swob import Request
-from swift.common.utils import quote, closing_if_possible
+from swift.common.utils import quote, closing_if_possible, \
+    server_handled_successfully
 from swift.common.wsgi import loadapp, pipeline_property
 
 if six.PY3:
@@ -191,11 +193,20 @@ class InternalClient(object):
                 req.params = params
             try:
                 resp = req.get_response(self.app)
+            except (Exception, Timeout):
+                exc_type, exc_value, exc_traceback = exc_info()
+            else:
                 if resp.status_int in acceptable_statuses or \
                         resp.status_int // 100 in acceptable_statuses:
                     return resp
-            except (Exception, Timeout):
-                exc_type, exc_value, exc_traceback = exc_info()
+                elif server_handled_successfully(resp.status_int):
+                    # No sense retrying when we expect the same result
+                    break
+                elif resp.status_int == HTTP_CONFLICT and 'x-timestamp' in [
+                        header.lower() for header in headers]:
+                    # Since the caller provided the timestamp, retrying won't
+                    # change the result
+                    break
             # sleep only between tries, not after each one
             if attempt < self.request_tries - 1:
                 if resp:
diff --git a/test/unit/common/test_internal_client.py b/test/unit/common/test_internal_client.py
index 0a6694b3c..fae36267b 100644
--- a/test/unit/common/test_internal_client.py
+++ b/test/unit/common/test_internal_client.py
@@ -462,48 +462,65 @@ class TestInternalClient(unittest.TestCase):
                 mock.patch('swift.common.internal_client.sleep'):
             client.make_request('DELETE', '/container', {}, (200,))
 
-        for logline in client.logger.get_lines_for_level('info'):
-            # add spaces before/after 409 because it can match with
-            # something like timestamp
-            self.assertIn(' 409 ', logline)
+        # Since we didn't provide an X-Timestamp, retrying gives us a chance to
+        # succeed (assuming the failure was due to clock skew between servers)
+        expected = (' HTTP/1.0 409 ', ' HTTP/1.0 409 ', ' HTTP/1.0 409 ', )
+        loglines = client.logger.get_lines_for_level('info')
+        for expected, logline in izip_longest(expected, loglines):
+            if not expected:
+                self.fail('Unexpected extra log line: %r' % logline)
+            self.assertIn(expected, logline)
 
     def test_make_request_acceptable_status_not_2xx(self):
-        closed_paths = []
-
-        def mark_closed(path):
-            closed_paths.append(path)
-
         class InternalClient(internal_client.InternalClient):
-            def __init__(self):
+            def __init__(self, resp_status):
                 self.logger = DebugLogger()
                 # wrap the fake app with ProxyLoggingMiddleware
                 self.app = ProxyLoggingMiddleware(
                     self.fake_app, {}, self.logger)
                 self.user_agent = 'some_agent'
+                self.resp_status = resp_status
                 self.request_tries = 3
+                self.closed_paths = []
 
             def fake_app(self, env, start_response):
                 body = 'fake error response'
-                start_response('200 Ok', [('Content-Length', str(len(body)))])
-                return LeakTrackingIter((c for c in body),
-                                        mark_closed, env['PATH_INFO'])
+                start_response(self.resp_status,
+                               [('Content-Length', str(len(body)))])
+                return LeakTrackingIter(body, self.closed_paths.append,
+                                        env['PATH_INFO'])
+
+        def do_test(resp_status):
+            client = InternalClient(resp_status)
+            with self.assertRaises(internal_client.UnexpectedResponse) as ctx, \
+                    mock.patch('swift.common.internal_client.sleep'):
+                # This is obvious strange tests to expect only 400 Bad Request
+                # but this test intended to avoid extra body drain if it's
+                # correct object body with 2xx.
+                client.make_request('GET', '/cont/obj', {}, (400,))
+            loglines = client.logger.get_lines_for_level('info')
+            return client.closed_paths, ctx.exception.resp, loglines
+
+        closed_paths, resp, loglines = do_test('200 OK')
+        # Since the 200 is considered "properly handled", it won't be retried
+        self.assertEqual(closed_paths, [])
+        # ...and it'll be on us (the caller) to close (for example, by using
+        # swob.Response's body property)
+        self.assertEqual(resp.body, 'fake error response')
+        self.assertEqual(closed_paths, ['/cont/obj'])
+
+        expected = (' HTTP/1.0 200 ', )
+        for expected, logline in izip_longest(expected, loglines):
+            if not expected:
+                self.fail('Unexpected extra log line: %r' % logline)
+            self.assertIn(expected, logline)
 
-        client = InternalClient()
-        with self.assertRaises(internal_client.UnexpectedResponse) as ctx, \
-                mock.patch('swift.common.internal_client.sleep'):
-            # This is obvious strange tests to expect only 400 Bad Request
-            # but this test intended to avoid extra body drain if it's
-            # correct object body with 2xx.
-            client.make_request('GET', '/cont/obj', {}, (400,))
-        self.assertEqual(closed_paths, ['/cont/obj'] * 2)
-        # swob's body resp property will call close
-        self.assertEqual(ctx.exception.resp.body, 'fake error response')
+        closed_paths, resp, loglines = do_test('503 Service Unavailable')
+        # But since 5xx is neither "properly handled" not likely to include
+        # a large body, it will be retried and responses will already be closed
         self.assertEqual(closed_paths, ['/cont/obj'] * 3)
 
-        # Retries like this will cause 499 because internal_client won't
-        # automatically consume (potentially large) 2XX responses.
-        expected = (' 499 ', ' 499 ', ' 200 ')
-        loglines = client.logger.get_lines_for_level('info')
+        expected = (' HTTP/1.0 503 ', ' HTTP/1.0 503 ', ' HTTP/1.0 503 ', )
         for expected, logline in izip_longest(expected, loglines):
             if not expected:
                 self.fail('Unexpected extra log line: %r' % logline)
@@ -562,30 +579,33 @@ class TestInternalClient(unittest.TestCase):
                 self.test.assertEqual(0, whence)
 
         class InternalClient(internal_client.InternalClient):
-            def __init__(self):
+            def __init__(self, status):
                 self.app = self.fake_app
                 self.user_agent = 'some_agent'
                 self.request_tries = 3
+                self.status = status
+                self.call_count = 0
 
             def fake_app(self, env, start_response):
-                start_response('404 Not Found', [('Content-Length', '0')])
+                self.call_count += 1
+                start_response(self.status, [('Content-Length', '0')])
                 return []
 
-        fobj = FileObject(self)
-        client = InternalClient()
+        def do_test(status, expected_calls):
+            fobj = FileObject(self)
+            client = InternalClient(status)
 
-        try:
-            old_sleep = internal_client.sleep
-            internal_client.sleep = not_sleep
-            try:
-                client.make_request('PUT', '/', {}, (2,), fobj)
-            except Exception as err:
-                pass
-            self.assertEqual(404, err.resp.status_int)
-        finally:
-            internal_client.sleep = old_sleep
+            with mock.patch.object(internal_client, 'sleep', not_sleep):
+                with self.assertRaises(Exception) as exc_mgr:
+                    client.make_request('PUT', '/', {}, (2,), fobj)
+                self.assertEqual(int(status[:3]),
+                                 exc_mgr.exception.resp.status_int)
+
+            self.assertEqual(client.call_count, fobj.seek_called)
+            self.assertEqual(client.call_count, expected_calls)
 
-        self.assertEqual(client.request_tries, fobj.seek_called)
+        do_test('404 Not Found', 1)
+        do_test('503 Service Unavailable', 3)
 
     def test_make_request_request_exception(self):
         class InternalClient(internal_client.InternalClient):
@@ -650,17 +670,15 @@ class TestInternalClient(unittest.TestCase):
         self.assertEqual(1, client.make_request_called)
 
     def test_get_metadata_invalid_status(self):
-        class FakeApp(object):
-
-            def __call__(self, environ, start_response):
-                start_response('404 Not Found', [('x-foo', 'bar')])
-                return ['nope']
-
         class InternalClient(internal_client.InternalClient):
             def __init__(self):
                 self.user_agent = 'test'
                 self.request_tries = 1
-                self.app = FakeApp()
+                self.app = self.fake_app
+
+            def fake_app(self, environ, start_response):
+                start_response('404 Not Found', [('x-foo', 'bar')])
+                return ['nope']
 
         client = InternalClient()
         self.assertRaises(internal_client.UnexpectedResponse,
diff --git a/test/unit/obj/test_expirer.py b/test/unit/obj/test_expirer.py
index 09254e310..7bf6e29bc 100644
--- a/test/unit/obj/test_expirer.py
+++ b/test/unit/obj/test_expirer.py
@@ -741,30 +741,30 @@ class TestObjectExpirer(TestCase):
                          got_env[0]['HTTP_X_IF_DELETE_AT'])
         self.assertEqual(got_env[0]['PATH_INFO'], '/v1/path/to/object name')
 
-    def test_delete_actual_object_raises_404(self):
+    def test_delete_actual_object_returns_expected_error(self):
+        def do_test(test_status):
+            calls = [0]
 
-        def fake_app(env, start_response):
-            start_response('404 Not Found', [('Content-Length', '0')])
-            return []
-
-        internal_client.loadapp = lambda *a, **kw: fake_app
-
-        x = expirer.ObjectExpirer({})
-        self.assertRaises(internal_client.UnexpectedResponse,
-                          x.delete_actual_object, '/path/to/object', '1234')
-
-    def test_delete_actual_object_raises_412(self):
+            def fake_app(env, start_response):
+                calls[0] += 1
+                start_response(test_status, [('Content-Length', '0')])
+                return []
 
-        def fake_app(env, start_response):
-            start_response('412 Precondition Failed',
-                           [('Content-Length', '0')])
-            return []
+            internal_client.loadapp = lambda *a, **kw: fake_app
 
-        internal_client.loadapp = lambda *a, **kw: fake_app
+            x = expirer.ObjectExpirer({})
+            self.assertRaises(internal_client.UnexpectedResponse,
+                              x.delete_actual_object, '/path/to/object',
+                              '1234')
+            self.assertEqual(calls[0], 1)
 
-        x = expirer.ObjectExpirer({})
-        self.assertRaises(internal_client.UnexpectedResponse,
-                          x.delete_actual_object, '/path/to/object', '1234')
+        # object was deleted and tombstone reaped
+        do_test('404 Not Found')
+        # object was overwritten *after* the original expiration, or
+        # object was deleted but tombstone still exists, or
+        # object was overwritten ahead of the original expiration, or
+        # object was POSTed to with a new (or no) expiration, or ...
+        do_test('412 Precondition Failed')
 
     def test_delete_actual_object_does_not_handle_odd_stuff(self):
 
@@ -790,7 +790,8 @@ class TestObjectExpirer(TestCase):
         name = 'this name should get quoted'
         timestamp = '1366063156.863045'
         x = expirer.ObjectExpirer({})
-        x.swift.make_request = mock.MagicMock()
+        x.swift.make_request = mock.Mock()
+        x.swift.make_request.return_value.status_int = 204
         x.delete_actual_object(name, timestamp)
         self.assertEqual(x.swift.make_request.call_count, 1)
         self.assertEqual(x.swift.make_request.call_args[0][1],