openstack/swift
Code Issues Proposed changes

Remove double GET on range requests.

Browse Source 
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
This commit is contained in:
Samuel Merritt 2012-10-24 17:16:11 -07:00
parent cee685e11a
commit 9cb7751fad
5 changed files with 29 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
swift/common/swob.py
View File

@ -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

10
swift/proxy/controllers/base.py
View File

@ -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)

32
swift/proxy/controllers/obj.py
View File

@ -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

4
test/unit/common/test_swob.py
View File

@ -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(

1
test/unit/proxy/test_server.py
View File

@ -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):