Fix socket leak on 416 EC GET responses.

Sometimes, when handling an EC GET request with a Range header, the
object servers reply 206 to the proxy, but the proxy (correctly)
replies 416 to the client[1]. In that case, the connections to the object
servers were not being closed. This was due to improper error handling
in ECAppIter.

Since ECAppIter is intended to be a WSGI iterable, it expects to have
its close() method called when the caller is done with it. In this
particular case, the caller (ECAppIter.kickoff()) was not calling
close() when an exception was raised. Now it is.

[1] consider a 4+2 EC policy with segment size 1024, an 20 byte
object, and a request with "Range: bytes=21-50". The proxy needs whole
fragments to decode, so it asks the object server for "Range:
bytes=0-255" [2], the object server says 206, and then the proxy
realizes that the client's request is unsatisfiable and tells the
client 416.

[2] segment size 1024 and 4 data fragments means the fragments have
size 1024 / 4 = 256, hence "bytes=0-255" asks for the first whole
fragment

Change-Id: Ide2edf8c449c97d45f48c2dbbbff7aebefa4b158
Closes-Bug: 1738804

swift/proxy/controllers/obj.py

@@ -1023,7 +1023,11 @@ class ECAppIter(object):
         """
         self.mime_boundary = resp.boundary
 
-        self.stashed_iter = reiterate(self._real_iter(req, resp.headers))
+        try:
+            self.stashed_iter = reiterate(self._real_iter(req, resp.headers))
+        except Exception:
+            self.close()
+            raise
 
         if self.learned_content_type is not None:
             resp.content_type = self.learned_content_type
@@ -2083,7 +2087,7 @@ class ECGetResponseCollection(object):
         Return the best bucket in the collection.
 
         The "best" bucket is the newest timestamp with sufficient getters, or
-        the closest to having a sufficient getters, unless it is bettered by a
+        the closest to having sufficient getters, unless it is bettered by a
         bucket with potential alternate nodes.
 
         :return: An instance of :class:`~ECGetResponseBucket` or None if there

test/unit/proxy/test_server.py

@@ -59,6 +59,7 @@ from test.unit.helpers import setup_servers, teardown_servers
 from swift.proxy import server as proxy_server
 from swift.proxy.controllers.obj import ReplicatedObjectController
 from swift.obj import server as object_server
+from swift.common.bufferedhttp import BufferedHTTPResponse
 from swift.common.middleware import proxy_logging, versioned_writes, \
     copy, listing_formats
 from swift.common.middleware.acl import parse_acl, format_acl
@@ -7168,6 +7169,48 @@ class TestObjectECRangedGET(unittest.TestCase):
         self.assertIn('Content-Range', headers)
         self.assertEqual('bytes */%d' % obj_len, headers['Content-Range'])
 
+    def test_unsatisfiable_socket_leak(self):
+        unclosed_http_responses = {}
+        tracked_responses = [0]
+
+        class LeakTrackingHTTPResponse(BufferedHTTPResponse):
+            def begin(self):
+                # no super(); we inherit from an old-style class (it's
+                # httplib's fault; don't try and fix it).
+                retval = BufferedHTTPResponse.begin(self)
+                if self.status != 204:
+                    # This mock is overly broad and catches account and
+                    # container HEAD requests too. We don't care about
+                    # those; it's the object GETs that were leaky.
+                    #
+                    # Unfortunately, we don't have access to the request
+                    # path here, so we use "status == 204" as a crude proxy
+                    # for "not an object response".
+                    unclosed_http_responses[id(self)] = self
+                    tracked_responses[0] += 1
+                return retval
+
+            def close(self, *args, **kwargs):
+                rv = BufferedHTTPResponse.close(self, *args, **kwargs)
+                unclosed_http_responses.pop(id(self), None)
+                return rv
+
+            def __repr__(self):
+                swift_conn = getattr(self, 'swift_conn', None)
+                method = getattr(swift_conn, '_method', '<unknown>')
+                path = getattr(swift_conn, '_path', '<unknown>')
+                return '%s<method=%r path=%r>' % (
+                    self.__class__.__name__, method, path)
+
+        obj_len = len(self.obj)
+        with mock.patch('swift.common.bufferedhttp.BufferedHTTPConnection'
+                        '.response_class', LeakTrackingHTTPResponse):
+            status, headers, _junk = self._get_obj(
+                "bytes=%d-%d" % (obj_len, obj_len + 100))
+        self.assertEqual(status, 416)  # sanity check
+        self.assertGreater(tracked_responses[0], 0)  # ensure tracking happened
+        self.assertEqual(unclosed_http_responses, {})
+
     def test_off_end(self):
         # Ranged GET that's mostly off the end of the object, but overlaps
         # it in just the last byte