Refactor internal APIs to use our own HTTPResponse

changelog/2648.feature.rst

@@ -0,0 +1,3 @@
+Modified ``urllib3.HTTPConnection.getresponse`` to return an instance of ``urllib3.HTTPResponse`` instead of ``http.client.HTTPResponse``.
+
+Removed ``urllib3.HTTPResponse.from_httplib``.

ci/0004-requests-chunked-requests.patch

@@ -0,0 +1,40 @@
+diff --git a/tests/test_lowlevel.py b/tests/test_lowlevel.py
+index 859d07e8..91fbce7c 100644
+--- a/tests/test_lowlevel.py
++++ b/tests/test_lowlevel.py
+@@ -20,7 +20,7 @@ def echo_response_handler(sock):
+     ) % (len(request_content), request_content)
+     sock.send(text_200)
+
+-
++@pytest.mark.xfail
+ def test_chunked_upload():
+     """can safely send generators"""
+     close_server = threading.Event()
+@@ -35,7 +35,7 @@ def test_chunked_upload():
+     assert r.status_code == 200
+     assert r.request.headers["Transfer-Encoding"] == "chunked"
+
+-
++@pytest.mark.xfail
+ def test_chunked_encoding_error():
+     """get a ChunkedEncodingError if the server returns a bad response"""
+
+@@ -59,7 +59,7 @@ def test_chunked_encoding_error():
+             requests.get(url)
+         close_server.set()  # release server block
+
+-
++@pytest.mark.xfail
+ def test_chunked_upload_uses_only_specified_host_header():
+     """Ensure we use only the specified Host header for chunked requests."""
+     close_server = threading.Event()
+@@ -77,7 +77,7 @@ def test_chunked_upload_uses_only_specified_host_header():
+     assert expected_header in r.content
+     assert r.content.count(b"Host: ") == 1
+
+-
++@pytest.mark.xfail
+ def test_chunked_upload_doesnt_skip_host_header():
+     """Ensure we don't omit all Host headers with chunked requests."""
+     close_server = threading.Event()

noxfile.py

@@ -116,7 +116,12 @@ def downstream_requests(session: nox.Session) -> None:
     session.cd(tmp_dir)
     git_clone(session, "https://github.com/psf/requests")
     session.chdir("requests")
-    session.run("git", "apply", f"{root}/ci/requests.patch", external=True)
+    session.run(
+        "git", "apply", f"{root}/ci/0003-requests-removed-warnings.patch", external=True
+    )
+    session.run(
+        "git", "apply", f"{root}/ci/0004-requests-chunked-requests.patch", external=True
+    )
     session.run("git", "rev-parse", "HEAD", external=True)
     session.install(".[socks]", silent=False)
     session.install("-r", "requirements-dev.txt", silent=False)

src/urllib3/_request_methods.py

@@ -85,8 +85,6 @@ def request(
         """
         method = method.upper()
 
-        urlopen_kw["request_url"] = url
-
         if json is not None and body is not None:
             raise TypeError(
                 "request got values for both 'body' and 'json' parameters which are mutually exclusive"

src/urllib3/connection.py

@@ -6,6 +6,7 @@
 import warnings
 from http.client import HTTPConnection as _HTTPConnection
 from http.client import HTTPException as HTTPException  # noqa: F401
+from http.client import ResponseNotReady
 from socket import timeout as SocketTimeout
 from typing import (
     IO,
@@ -24,9 +25,12 @@
 if TYPE_CHECKING:
     from typing_extensions import Literal
 
+    from .response import HTTPResponse
     from .util.ssl_ import _TYPE_PEER_CERT_RET_DICT
     from .util.ssltransport import SSLTransport
 
+from ._collections import HTTPHeaderDict
+from .util.response import assert_header_parsing
 from .util.timeout import _DEFAULT_TIMEOUT, _TYPE_TIMEOUT, Timeout
 from .util.util import to_str
 
@@ -44,6 +48,7 @@ class BaseSSLError(BaseException):  # type: ignore[no-redef]
 from ._version import __version__
 from .exceptions import (
     ConnectTimeoutError,
+    HeaderParsingError,
     NameResolutionError,
     NewConnectionError,
     ProxyError,
@@ -77,7 +82,6 @@ class BaseSSLError(BaseException):  # type: ignore[no-redef]
 
 _CONTAINS_CONTROL_CHAR_RE = re.compile(r"[^-!#$%&'*+.^_`|~0-9a-zA-Z]")
 
-
 _TYPE_BODY = Union[bytes, IO[Any], Iterable[bytes], str]
 
 
@@ -88,6 +92,16 @@ class ProxyConfig(NamedTuple):
     assert_fingerprint: Optional[str]
 
 
+class _ResponseOptions(NamedTuple):
+    # TODO: Remove this in favor of a better
+    # HTTP request/response lifecycle tracking.
+    request_method: str
+    request_url: str
+    preload_content: bool
+    decode_content: bool
+    enforce_content_length: bool
+
+
 class HTTPConnection(_HTTPConnection):
     """
     Based on :class:`http.client.HTTPConnection` but provides an extra constructor
@@ -134,6 +148,7 @@ class HTTPConnection(_HTTPConnection):
     _tunnel_host: Optional[str]
     _tunnel: Callable[["HTTPConnection"], None]
     _connecting_to_proxy: bool
+    _response_options: Optional[_ResponseOptions]
 
     def __init__(
         self,
@@ -166,6 +181,7 @@ def __init__(
         )
 
         self._connecting_to_proxy = False
+        self._response_options = None
 
     # https://github.com/python/mypy/issues/4125
     # Mypy treats this as LSP violation, which is considered a bug.
@@ -292,8 +308,27 @@ def request(  # type: ignore[override]
         body: Optional[_TYPE_BODY] = None,
         headers: Optional[Mapping[str, str]] = None,
         chunked: bool = False,
+        preload_content: bool = True,
+        decode_content: bool = True,
+        enforce_content_length: bool = True,
     ) -> None:
 
+        # Store these values to be fed into the HTTPResponse
+        # object later. TODO: Remove this in favor of a real
+        # HTTP lifecycle mechanism.
+
+        # We have to store these before we call .request()
+        # because sometimes we can still salvage a response
+        # off the wire even if we aren't able to completely
+        # send the request body.
+        self._response_options = _ResponseOptions(
+            request_method=method,
+            request_url=url,
+            preload_content=preload_content,
+            decode_content=decode_content,
+            enforce_content_length=enforce_content_length,
+        )
+
         if headers is None:
             headers = {}
         header_keys = frozenset(to_str(k.lower()) for k in headers)
@@ -371,6 +406,57 @@ def request_chunked(
         """
         self.request(method, url, body=body, headers=headers, chunked=True)
 
+    def getresponse(  # type: ignore[override]
+        self,
+    ) -> "HTTPResponse":
+        """
+        Get the response from the server.
+
+        If the HTTPConnection is in the correct state, returns an instance of HTTPResponse or of whatever object is returned by the response_class variable.
+
+        If a request has not been sent or if a previous response has not be handled, ResponseNotReady is raised. If the HTTP response indicates that the connection should be closed, then it will be closed before the response is returned. When the connection is closed, the underlying socket is closed.
+        """
+        # Raise the same error as http.client.HTTPConnection
+        if self._response_options is None:
+            raise ResponseNotReady()
+
+        # Reset this attribute for being used again.
+        resp_options = self._response_options
+        self._response_options = None
+
+        # This is needed here to avoid circular import errors
+        from .response import HTTPResponse
+
+        # Get the response from http.client.HTTPConnection
+        httplib_response = super().getresponse()
+
+        try:
+            assert_header_parsing(httplib_response.msg)
+        except (HeaderParsingError, TypeError) as hpe:
+            log.warning(
+                "Failed to parse headers (url=%s): %s",
+                _url_from_connection(self, resp_options.request_url),
+                hpe,
+                exc_info=True,
+            )
+
+        headers = HTTPHeaderDict(httplib_response.msg.items())
+
+        response = HTTPResponse(
+            body=httplib_response,
+            headers=headers,
+            status=httplib_response.status,
+            version=httplib_response.version,
+            reason=httplib_response.reason,
+            preload_content=resp_options.preload_content,
+            decode_content=resp_options.decode_content,
+            original_response=httplib_response,
+            enforce_content_length=resp_options.enforce_content_length,
+            request_method=resp_options.request_method,
+            request_url=resp_options.request_url,
+        )
+        return response
+
 
 class HTTPSConnection(HTTPConnection):
     """
@@ -748,3 +834,13 @@ class DummyConnection:
 
 
 VerifiedHTTPSConnection = HTTPSConnection
+
+
+def _url_from_connection(
+    conn: Union[HTTPConnection, HTTPSConnection], path: Optional[str] = None
+) -> str:
+    """Returns the URL from a given connection. This is mainly used for testing and logging."""
+
+    scheme = "https" if isinstance(conn, HTTPSConnection) else "http"
+
+    return Url(scheme=scheme, host=conn.host, port=conn.port, path=path).url