From 9cb7751fad92bc237b62710b6f4926db69cb05bd Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Wed, 24 Oct 2012 17:16:11 -0700 Subject: [PATCH] Remove double GET on range requests. The proxy server's ObjectController was performing multiple GET requests to the object server when processing requests with Range headers. This was a workaround for a bug in the proxy server's Controller.GETorHEAD_base method where a response code of 416 from the object server was incorrectly treated as a bad response from the backend, the same way a 404 or a 5xx would be. A 416 (Requested Range Not Satisfiable) from the object server is now considered a good response. Since the response headers from the object server already include the X-Object-Manifest header, there's no need to make a second request (sans Range header) to see if the object is a manifest. Bonus fix: updated message for status 416 to match RFC2616. Bonus fix 2: removed a leftover debugging print() in test/unit/proxy/test_server.py. Fixes bug 1065869. Change-Id: I156af0b463f02ef19a8cfe37092544a599d89b78 --- swift/common/swob.py | 8 +++----- swift/proxy/controllers/base.py | 10 ++++++++-- swift/proxy/controllers/obj.py | 32 ++++++++++++++++---------------- test/unit/common/test_swob.py | 4 ++-- test/unit/proxy/test_server.py | 1 - 5 files changed, 29 insertions(+), 26 deletions(-) diff --git a/swift/common/swob.py b/swift/common/swob.py index 3eefe0a82c..079868918a 100644 --- a/swift/common/swob.py +++ b/swift/common/swob.py @@ -70,7 +70,7 @@ RESPONSE_REASONS = { 'server.'), 415: ('Unsupported Media Type', 'The request media type is not ' 'supported by this server.'), - 416: ('Request Range Not Satisfiable', 'The Range requested is not ' + 416: ('Requested Range Not Satisfiable', 'The Range requested is not ' 'available.'), 417: ('Expectation Failed', 'Expectation failed.'), 422: ('Unprocessable Entity', 'Unable to process the contained ' @@ -271,8 +271,6 @@ def _resp_status_property(): When set to a str, it splits status_int and title apart. When set to an integer, retrieves the correct title for that response code from the RESPONSE_REASONS dict. - - :param header: name of the header, e.g. "Content-Length" """ def getter(self): return '%s %s' % (self.status_int, self.title) @@ -383,8 +381,8 @@ def _resp_charset_property(): def _resp_app_iter_property(): """ Set and retrieve Response.app_iter - Mostly a pass-through to Response._app_iter, it's a property so it can zero - out an exsisting content-length on assignment. + Mostly a pass-through to Response._app_iter; it's a property so it can zero + out an existing content-length on assignment. """ def getter(self): return self._app_iter diff --git a/swift/proxy/controllers/base.py b/swift/proxy/controllers/base.py index 12196a2de5..fda2a2764c 100644 --- a/swift/proxy/controllers/base.py +++ b/swift/proxy/controllers/base.py @@ -566,6 +566,13 @@ class Controller(object): except Exception: pass + def is_good_source(self, src): + """ + Indicates whether or not the request made to the backend found + what it was looking for. + """ + return is_success(src.status) or is_redirection(src.status) + def GETorHEAD_base(self, req, server_type, partition, nodes, path, attempts): """ @@ -609,8 +616,7 @@ class Controller(object): node, server_type, _('Trying to %(method)s %(path)s') % {'method': req.method, 'path': req.path}) continue - if is_success(possible_source.status) or \ - is_redirection(possible_source.status): + if self.is_good_source(possible_source): # 404 if we know we don't have a synced copy if not float(possible_source.getheader('X-PUT-Timestamp', 1)): statuses.append(HTTP_NOT_FOUND) diff --git a/swift/proxy/controllers/obj.py b/swift/proxy/controllers/obj.py index 712a7c3f2a..c003c26fb6 100644 --- a/swift/proxy/controllers/obj.py +++ b/swift/proxy/controllers/obj.py @@ -276,6 +276,17 @@ class ObjectController(Controller): for obj in sublisting: yield obj + def is_good_source(self, src): + """ + Indicates whether or not the request made to the backend found + what it was looking for. + + In the case of an object, a 416 indicates that we found a + backend with the object. + """ + return src.status == 416 or \ + super(ObjectController, self).is_good_source(src) + def GETorHEAD(self, req): """Handle HTTP GET or HEAD requests.""" container_info = self.container_info(self.account_name, @@ -285,28 +296,13 @@ class ObjectController(Controller): aresp = req.environ['swift.authorize'](req) if aresp: return aresp + partition, nodes = self.app.object_ring.get_nodes( self.account_name, self.container_name, self.object_name) shuffle(nodes) resp = self.GETorHEAD_base(req, _('Object'), partition, self.iter_nodes(partition, nodes, self.app.object_ring), req.path_info, len(nodes)) - # Whether we get a 416 Requested Range Not Satisfiable or not, - # we should request a manifest because size of manifest file - # can be not 0. After checking a manifest, redo the range request - # on the whole object. - if req.range: - req_range = req.range - req.range = None - resp2 = self.GETorHEAD_base(req, _('Object'), partition, - self.iter_nodes(partition, - nodes, - self.app.object_ring), - req.path_info, len(nodes)) - if 'x-object-manifest' not in resp2.headers: - return resp - resp = resp2 - req.range = str(req_range) if 'x-object-manifest' in resp.headers: lcontainer, lprefix = \ @@ -367,6 +363,10 @@ class ObjectController(Controller): resp.last_modified = last_modified resp.etag = etag resp.headers['accept-ranges'] = 'bytes' + # In case of a manifest file of nonzero length, the + # backend may have sent back a Content-Range header for + # the manifest. It's wrong for the client, though. + resp.content_range = None return resp diff --git a/test/unit/common/test_swob.py b/test/unit/common/test_swob.py index 18831f0d1d..603402ee72 100644 --- a/test/unit/common/test_swob.py +++ b/test/unit/common/test_swob.py @@ -419,14 +419,14 @@ class TestResponse(unittest.TestCase): body = ''.join(resp([], start_response)) self.assertEquals(body, '') self.assertEquals(resp.content_length, 0) - self.assertEquals(resp.status, '416 Request Range Not Satisfiable') + self.assertEquals(resp.status, '416 Requested Range Not Satisfiable') resp = swift.common.swob.Response( body='1234567890', request=req, conditional_response=True) body = ''.join(resp([], start_response)) self.assertEquals(body, '') - self.assertEquals(resp.status, '416 Request Range Not Satisfiable') + self.assertEquals(resp.status, '416 Requested Range Not Satisfiable') # Syntactically-invalid Range headers "MUST" be ignored req = swift.common.swob.Request.blank( diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index 62113c1789..59220aca59 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -1495,7 +1495,6 @@ class TestObjectController(unittest.TestCase): 'container', 'object') self.assert_status_map(controller.HEAD, (200, 200, 503, 200, 200), 200) - print controller.app.object_ring.devs self.assertEquals(controller.app.object_ring.devs[0]['errors'], 2) self.assert_('last_error' in controller.app.object_ring.devs[0]) for _junk in xrange(self.app.error_suppression_limit):