Connections which fail should not remain in the pool (#433)

Author: lovelydinosaur

httpcore/_async/connection.py

@@ -52,6 +52,7 @@ def __init__(
             AutoBackend() if network_backend is None else network_backend
         )
         self._connection: Optional[AsyncConnectionInterface] = None
+        self._connect_failed: bool = False
         self._request_lock = AsyncLock()
 
     async def handle_async_request(self, request: Request) -> Response:
@@ -62,27 +63,31 @@ async def handle_async_request(self, request: Request) -> Response:
 
         async with self._request_lock:
             if self._connection is None:
-                stream = await self._connect(request)
-
-                ssl_object = stream.get_extra_info("ssl_object")
-                http2_negotiated = (
-                    ssl_object is not None
-                    and ssl_object.selected_alpn_protocol() == "h2"
-                )
-                if http2_negotiated or (self._http2 and not self._http1):
-                    from .http2 import AsyncHTTP2Connection
-
-                    self._connection = AsyncHTTP2Connection(
-                        origin=self._origin,
-                        stream=stream,
-                        keepalive_expiry=self._keepalive_expiry,
-                    )
-                else:
-                    self._connection = AsyncHTTP11Connection(
-                        origin=self._origin,
-                        stream=stream,
-                        keepalive_expiry=self._keepalive_expiry,
+                try:
+                    stream = await self._connect(request)
+
+                    ssl_object = stream.get_extra_info("ssl_object")
+                    http2_negotiated = (
+                        ssl_object is not None
+                        and ssl_object.selected_alpn_protocol() == "h2"
                     )
+                    if http2_negotiated or (self._http2 and not self._http1):
+                        from .http2 import AsyncHTTP2Connection
+
+                        self._connection = AsyncHTTP2Connection(
+                            origin=self._origin,
+                            stream=stream,
+                            keepalive_expiry=self._keepalive_expiry,
+                        )
+                    else:
+                        self._connection = AsyncHTTP11Connection(
+                            origin=self._origin,
+                            stream=stream,
+                            keepalive_expiry=self._keepalive_expiry,
+                        )
+                except Exception as exc:
+                    self._connect_failed = True
+                    raise exc
             elif not self._connection.is_available():
                 raise ConnectionNotAvailable()
 
@@ -154,27 +159,31 @@ def is_available(self) -> bool:
             # If HTTP/2 support is enabled, and the resulting connection could
             # end up as HTTP/2 then we should indicate the connection as being
             # available to service multiple requests.
-            return self._http2 and (self._origin.scheme == b"https" or not self._http1)
+            return (
+                self._http2
+                and (self._origin.scheme == b"https" or not self._http1)
+                and not self._connect_failed
+            )
         return self._connection.is_available()
 
     def has_expired(self) -> bool:
         if self._connection is None:
-            return False
+            return self._connect_failed
         return self._connection.has_expired()
 
     def is_idle(self) -> bool:
         if self._connection is None:
-            return False
+            return self._connect_failed
         return self._connection.is_idle()
 
     def is_closed(self) -> bool:
         if self._connection is None:
-            return False
+            return self._connect_failed
         return self._connection.is_closed()
 
     def info(self) -> str:
         if self._connection is None:
-            return "CONNECTING"
+            return "CONNECTION FAILED" if self._connect_failed else "CONNECTING"
         return self._connection.info()
 
     def __repr__(self) -> str:

httpcore/_sync/connection.py

@@ -52,6 +52,7 @@ def __init__(
             SyncBackend() if network_backend is None else network_backend
         )
         self._connection: Optional[ConnectionInterface] = None
+        self._connect_failed: bool = False
         self._request_lock = Lock()
 
     def handle_request(self, request: Request) -> Response:
@@ -62,27 +63,31 @@ def handle_request(self, request: Request) -> Response:
 
         with self._request_lock:
             if self._connection is None:
-                stream = self._connect(request)
-
-                ssl_object = stream.get_extra_info("ssl_object")
-                http2_negotiated = (
-                    ssl_object is not None
-                    and ssl_object.selected_alpn_protocol() == "h2"
-                )
-                if http2_negotiated or (self._http2 and not self._http1):
-                    from .http2 import HTTP2Connection
-
-                    self._connection = HTTP2Connection(
-                        origin=self._origin,
-                        stream=stream,
-                        keepalive_expiry=self._keepalive_expiry,
-                    )
-                else:
-                    self._connection = HTTP11Connection(
-                        origin=self._origin,
-                        stream=stream,
-                        keepalive_expiry=self._keepalive_expiry,
+                try:
+                    stream = self._connect(request)
+
+                    ssl_object = stream.get_extra_info("ssl_object")
+                    http2_negotiated = (
+                        ssl_object is not None
+                        and ssl_object.selected_alpn_protocol() == "h2"
                     )
+                    if http2_negotiated or (self._http2 and not self._http1):
+                        from .http2 import HTTP2Connection
+
+                        self._connection = HTTP2Connection(
+                            origin=self._origin,
+                            stream=stream,
+                            keepalive_expiry=self._keepalive_expiry,
+                        )
+                    else:
+                        self._connection = HTTP11Connection(
+                            origin=self._origin,
+                            stream=stream,
+                            keepalive_expiry=self._keepalive_expiry,
+                        )
+                except Exception as exc:
+                    self._connect_failed = True
+                    raise exc
             elif not self._connection.is_available():
                 raise ConnectionNotAvailable()
 
@@ -154,27 +159,31 @@ def is_available(self) -> bool:
             # If HTTP/2 support is enabled, and the resulting connection could
             # end up as HTTP/2 then we should indicate the connection as being
             # available to service multiple requests.
-            return self._http2 and (self._origin.scheme == b"https" or not self._http1)
+            return (
+                self._http2
+                and (self._origin.scheme == b"https" or not self._http1)
+                and not self._connect_failed
+            )
         return self._connection.is_available()
 
     def has_expired(self) -> bool:
         if self._connection is None:
-            return False
+            return self._connect_failed
         return self._connection.has_expired()
 
     def is_idle(self) -> bool:
         if self._connection is None:
-            return False
+            return self._connect_failed
         return self._connection.is_idle()
 
     def is_closed(self) -> bool:
         if self._connection is None:
-            return False
+            return self._connect_failed
         return self._connection.is_closed()
 
     def info(self) -> str:
         if self._connection is None:
-            return "CONNECTING"
+            return "CONNECTION FAILED" if self._connect_failed else "CONNECTING"
         return self._connection.info()
 
     def __repr__(self) -> str:

tests/_async/test_connection_pool.py

@@ -3,7 +3,7 @@
 import pytest
 import trio as concurrency
 
-from httpcore import AsyncConnectionPool, UnsupportedProtocol
+from httpcore import AsyncConnectionPool, ConnectError, UnsupportedProtocol
 from httpcore.backends.mock import AsyncMockBackend
 
 
@@ -154,10 +154,10 @@ async def trace(name, kwargs):
 
 
 @pytest.mark.anyio
-async def test_connection_pool_with_exception():
+async def test_connection_pool_with_http_exception():
     """
-    HTTP/1.1 requests that result in an exception should not be returned to the
-    connection pool.
+    HTTP/1.1 requests that result in an exception during the connection should
+    not be returned to the connection pool.
     """
     network_backend = AsyncMockBackend([b"Wait, this isn't valid HTTP!"])
 
@@ -192,6 +192,42 @@ async def trace(name, kwargs):
     ]
 
 
+@pytest.mark.anyio
+async def test_connection_pool_with_connect_exception():
+    """
+    HTTP/1.1 requests that result in an exception during connection should not
+    be returned to the connection pool.
+    """
+
+    class FailedConnectBackend(AsyncMockBackend):
+        async def connect_tcp(
+            self, host: str, port: int, timeout: float = None, local_address: str = None
+        ):
+            raise ConnectError("Could not connect")
+
+    network_backend = FailedConnectBackend([])
+
+    called = []
+
+    async def trace(name, kwargs):
+        called.append(name)
+
+    async with AsyncConnectionPool(network_backend=network_backend) as pool:
+        # Sending an initial request, which once complete will not return to the pool.
+        with pytest.raises(Exception):
+            await pool.request(
+                "GET", "https://example.com/", extensions={"trace": trace}
+            )
+
+        info = [repr(c) for c in pool.connections]
+        assert info == []
+
+    assert called == [
+        "connection.connect_tcp.started",
+        "connection.connect_tcp.failed",
+    ]
+
+
 @pytest.mark.anyio
 async def test_connection_pool_with_immediate_expiry():
     """

tests/_sync/test_connection_pool.py

@@ -3,7 +3,7 @@
 import pytest
 from tests import concurrency
 
-from httpcore import ConnectionPool, UnsupportedProtocol
+from httpcore import ConnectionPool, ConnectError, UnsupportedProtocol
 from httpcore.backends.mock import MockBackend
 
 
@@ -154,10 +154,10 @@ def trace(name, kwargs):
 
 
 
-def test_connection_pool_with_exception():
+def test_connection_pool_with_http_exception():
     """
-    HTTP/1.1 requests that result in an exception should not be returned to the
-    connection pool.
+    HTTP/1.1 requests that result in an exception during the connection should
+    not be returned to the connection pool.
     """
     network_backend = MockBackend([b"Wait, this isn't valid HTTP!"])
 
@@ -193,6 +193,42 @@ def trace(name, kwargs):
 
 
 
+def test_connection_pool_with_connect_exception():
+    """
+    HTTP/1.1 requests that result in an exception during connection should not
+    be returned to the connection pool.
+    """
+
+    class FailedConnectBackend(MockBackend):
+        def connect_tcp(
+            self, host: str, port: int, timeout: float = None, local_address: str = None
+        ):
+            raise ConnectError("Could not connect")
+
+    network_backend = FailedConnectBackend([])
+
+    called = []
+
+    def trace(name, kwargs):
+        called.append(name)
+
+    with ConnectionPool(network_backend=network_backend) as pool:
+        # Sending an initial request, which once complete will not return to the pool.
+        with pytest.raises(Exception):
+            pool.request(
+                "GET", "https://example.com/", extensions={"trace": trace}
+            )
+
+        info = [repr(c) for c in pool.connections]
+        assert info == []
+
+    assert called == [
+        "connection.connect_tcp.started",
+        "connection.connect_tcp.failed",
+    ]
+
+
+
 def test_connection_pool_with_immediate_expiry():
     """
     Connection pools with keepalive_expiry=0.0 should immediately expire