From e6d1796df3fa53ac12ea2d783a41a5d76ff03f2e Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Tue, 8 Dec 2015 16:36:05 -0800 Subject: [PATCH] Fix socket leak in proxy on truncated SLO/DLO GET When a client disconnected while consuming an SLO or DLO GET response, the proxy would leak a socket. This could be observed via strace as a socket that had shutdown() called on it, but was never closed. It could also be observed by counting entries in /proc//fd, where is the pid of a proxy server worker process. The ultimate cause was that a WSGI iterable was receiving a close() message, but was not propagating that to the internal WSGI iterables that it owned. In this case, the culprit was swift.common.request_helpers.SegmentedIterable. The fix is, of course, to propagate the close() message so that the internal WSGI iterables can clean up after themselves. Co-Authored-By: Kota Tsuyuzaki Change-Id: Ib86c4c45641485ce1034212bf6f53bb84f02f612 --- swift/common/request_helpers.py | 6 ++++-- test/unit/common/middleware/helpers.py | 17 ++++++++++++++--- test/unit/common/middleware/test_slo.py | 29 +++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/swift/common/request_helpers.py b/swift/common/request_helpers.py index f12f876..d42356b 100644 --- a/swift/common/request_helpers.py +++ b/swift/common/request_helpers.py @@ -454,6 +454,9 @@ class SegmentedIterable(object): self.logger.exception(_('ERROR: An error occurred ' 'while retrieving segments')) raise + finally: + if self.current_resp: + close_if_possible(self.current_resp.app_iter) def app_iter_range(self, *a, **kw): """ @@ -496,5 +499,4 @@ class SegmentedIterable(object): Called when the client disconnect. Ensure that the connection to the backend server is closed. """ - if self.current_resp: - close_if_possible(self.current_resp.app_iter) + close_if_possible(self.app_iter) diff --git a/test/unit/common/middleware/helpers.py b/test/unit/common/middleware/helpers.py index bc6ad50..2308afd 100644 --- a/test/unit/common/middleware/helpers.py +++ b/test/unit/common/middleware/helpers.py @@ -15,6 +15,7 @@ # This stuff can't live in test/unit/__init__.py due to its swob dependency. +import six from collections import defaultdict from copy import deepcopy from hashlib import md5 @@ -117,10 +118,20 @@ class FakeSwift(object): if "CONTENT_TYPE" in env: self.uploaded[path][0]['Content-Type'] = env["CONTENT_TYPE"] - # range requests ought to work, hence conditional_response=True req = swob.Request(env) - resp = resp_class(req=req, headers=headers, body=body, - conditional_response=True) + if body is None: + body = '' + if isinstance(body, six.string_types): + # range requests ought to work, hence conditional_response=True + resp = resp_class(req=req, headers=headers, body=body, + conditional_response=True) + else: + # NB: range requests probably won't work with a non-string body + # unless whatever fancy iterable thingy that we've got here + # supports app_iter_range[s]. + resp = resp_class(req=req, headers=headers, + app_iter=body, + conditional_response=True) wsgi_iter = resp(env, start_response) self.mark_opened(path) return LeakTrackingIter(wsgi_iter, self, path) diff --git a/test/unit/common/middleware/test_slo.py b/test/unit/common/middleware/test_slo.py index 32d4954..afb76a7 100644 --- a/test/unit/common/middleware/test_slo.py +++ b/test/unit/common/middleware/test_slo.py @@ -1944,6 +1944,35 @@ class TestSloGetManifest(SloTestCase): self.assertEqual(headers['X-Object-Meta-Fish'], 'Bass') self.assertEqual(body, '') + def test_closing_first_segment(self): + req = Request.blank( + '/v1/AUTH_test/gettest/manifest-abcd', + environ={'REQUEST_METHOD': 'GET', + 'HTTP_ACCEPT': 'application/json'}) + + self.app.register( + 'GET', '/v1/AUTH_test/gettest/a_5', + swob.HTTPOk, {'Content-Length': '5', + 'Etag': md5hex('a' * 5)}, + ['a', 'a', 'a', 'a', 'a']) + + status = [None] + headers = [None] + + def start_response(s, h, ei=None): + status[0] = s + headers[0] = h + + app_resp = self.slo(req.environ, start_response) + body_iter = iter(app_resp) + chunk = next(body_iter) + self.assertEqual(chunk, 'a') # sanity check + + app_resp.close() + # yes, this duplicates the logic in tearDown(), but it makes the + # intention of this test clear + self.assertEqual(self.app.unclosed_requests, {}) + def test_head_manifest_is_efficient(self): req = Request.blank( '/v1/AUTH_test/gettest/manifest-abcd', -- 2.6.3