From 50ba8e69755e99bc66e8a335591ee4a0393fe8fb Mon Sep 17 00:00:00 2001
From: Tim Burke <tim.burke@gmail.com>
Date: Tue, 15 Jun 2021 14:41:01 -0700
Subject: [PATCH] bufferedhttp: Tolerate socket being None

Seen in prod:

Traceback (most recent call last):
  File ".../swift/proxy/controllers/obj.py", line 416, in _get_conn_response
    self.app.node_timeout, not final_phase)
  File ".../swift/proxy/controllers/obj.py", line 1645, in await_response
    self.resp = self.conn.getexpect()
  File ".../swift/common/bufferedhttp.py", line 229, in getexpect
    response = BufferedHTTPResponse(self.sock, **kwargs)
  File ".../swift/common/bufferedhttp.py", line 64, in __init__
    self._real_socket = sock.fd._sock
AttributeError: 'NoneType' object has no attribute 'fd'

Change-Id: I35b3660f954bcf91aee2698b2c9cc10d5447abc1
Co-Authored-By: Clay Gerrard <clay.gerrard@gmail.com>
---
 swift/common/bufferedhttp.py          | 18 ++++++++---
 test/unit/common/test_bufferedhttp.py | 44 +++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/swift/common/bufferedhttp.py b/swift/common/bufferedhttp.py
index a9633e34c2..c0aa259b3d 100644
--- a/swift/common/bufferedhttp.py
+++ b/swift/common/bufferedhttp.py
@@ -33,7 +33,7 @@ import socket
 
 import eventlet
 from eventlet.green.httplib import CONTINUE, HTTPConnection, HTTPMessage, \
-    HTTPResponse, HTTPSConnection, _UNKNOWN
+    HTTPResponse, HTTPSConnection, _UNKNOWN, ImproperConnectionState
 from six.moves.urllib.parse import quote, parse_qsl, urlencode
 import six
 
@@ -56,16 +56,24 @@ class BufferedHTTPResponse(HTTPResponse):
 
     def __init__(self, sock, debuglevel=0, strict=0,
                  method=None):          # pragma: no cover
+        # sock should be an eventlet.greenio.GreenSocket
         self.sock = sock
-        # sock is an eventlet.greenio.GreenSocket
-        if six.PY2:
+        if sock is None:
+            # ...but it could be None if we close the connection as we're
+            # getting it wrapped up in a Response
+            self._real_socket = None
+            # No socket means no file-like -- set it to None like in
+            # HTTPResponse.close()
+            self.fp = None
+        elif six.PY2:
             # sock.fd is a socket._socketobject
             # sock.fd._sock is a _socket.socket object, which is what we want.
             self._real_socket = sock.fd._sock
+            self.fp = sock.makefile('rb')
         else:
             # sock.fd is a socket.socket, which should have a _real_close
             self._real_socket = sock.fd
-        self.fp = sock.makefile('rb')
+            self.fp = sock.makefile('rb')
         self.debuglevel = debuglevel
         self.strict = strict
         self._method = method
@@ -106,6 +114,8 @@ class BufferedHTTPResponse(HTTPResponse):
         if self.fp:
             self.fp.close()
             self.fp = None
+        if not self.sock:
+            raise ImproperConnectionState('Socket already closed')
         self.fp = self.sock.makefile('rb', 0)
         version, status, reason = self._read_status()
         if status != CONTINUE:
diff --git a/test/unit/common/test_bufferedhttp.py b/test/unit/common/test_bufferedhttp.py
index 38e62856cd..cb5547cb38 100644
--- a/test/unit/common/test_bufferedhttp.py
+++ b/test/unit/common/test_bufferedhttp.py
@@ -99,6 +99,50 @@ class TestBufferedHTTP(unittest.TestCase):
                 if err:
                     raise Exception(err)
 
+    def test_get_expect(self):
+        bindsock = listen_zero()
+        request = []
+
+        def accept():
+            with Timeout(3):
+                sock, addr = bindsock.accept()
+                fp = sock.makefile('rwb')
+                request.append(fp.readline())
+                fp.write(b'HTTP/1.1 100 Continue\r\n\r\n')
+                fp.flush()
+                fp.write(b'HTTP/1.1 200 OK\r\nContent-Length: 8\r\n\r\n'
+                         b'RESPONSE')
+                fp.flush()
+
+        server = spawn(accept)
+        try:
+            address = '%s:%s' % ('127.0.0.1', bindsock.getsockname()[1])
+            conn = bufferedhttp.BufferedHTTPConnection(address)
+            conn.putrequest('GET', '/path')
+            conn.endheaders()
+            resp = conn.getexpect()
+            self.assertIsInstance(resp, bufferedhttp.BufferedHTTPResponse)
+            self.assertEqual(resp.status, 100)
+            self.assertEqual(resp.version, 11)
+            self.assertEqual(resp.reason, 'Continue')
+            # I don't think you're supposed to "read" a continue response
+            self.assertRaises(AssertionError, resp.read)
+
+            resp = conn.getresponse()
+            self.assertIsInstance(resp, bufferedhttp.BufferedHTTPResponse)
+            self.assertEqual(resp.read(), b'RESPONSE')
+
+        finally:
+            server.wait()
+        self.assertEqual(request[0], b'GET /path HTTP/1.1\r\n')
+
+    def test_closed_response(self):
+        resp = bufferedhttp.BufferedHTTPResponse(None)
+        self.assertEqual(resp.status, 'UNKNOWN')
+        self.assertEqual(resp.version, 'UNKNOWN')
+        self.assertEqual(resp.reason, 'UNKNOWN')
+        self.assertEqual(resp.read(), b'')
+
     def test_nonstr_header_values(self):
         with mock.patch('swift.common.bufferedhttp.HTTPSConnection',
                         MockHTTPSConnection):