Skip to content

Commit ccf15ef

Browse files
tomchristietomchristie
authored
Connections which fail should not remain in the pool (#433)
1 parent 1ebb420 commit ccf15ef

File tree

4 files changed

+148
-58
lines changed

4 files changed

+148
-58
lines changed

Diff for: ‎httpcore/_async/connection.py

+34-25
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ def __init__(
5252
AutoBackend() if network_backend is None else network_backend
5353
)
5454
self._connection: Optional[AsyncConnectionInterface] = None
55+
self._connect_failed: bool = False
5556
self._request_lock = AsyncLock()
5657

5758
async def handle_async_request(self, request: Request) -> Response:
@@ -62,27 +63,31 @@ async def handle_async_request(self, request: Request) -> Response:
6263

6364
async with self._request_lock:
6465
if self._connection is None:
65-
stream = await self._connect(request)
66-
67-
ssl_object = stream.get_extra_info("ssl_object")
68-
http2_negotiated = (
69-
ssl_object is not None
70-
and ssl_object.selected_alpn_protocol() == "h2"
71-
)
72-
if http2_negotiated or (self._http2 and not self._http1):
73-
from .http2 import AsyncHTTP2Connection
74-
75-
self._connection = AsyncHTTP2Connection(
76-
origin=self._origin,
77-
stream=stream,
78-
keepalive_expiry=self._keepalive_expiry,
79-
)
80-
else:
81-
self._connection = AsyncHTTP11Connection(
82-
origin=self._origin,
83-
stream=stream,
84-
keepalive_expiry=self._keepalive_expiry,
66+
try:
67+
stream = await self._connect(request)
68+
69+
ssl_object = stream.get_extra_info("ssl_object")
70+
http2_negotiated = (
71+
ssl_object is not None
72+
and ssl_object.selected_alpn_protocol() == "h2"
8573
)
74+
if http2_negotiated or (self._http2 and not self._http1):
75+
from .http2 import AsyncHTTP2Connection
76+
77+
self._connection = AsyncHTTP2Connection(
78+
origin=self._origin,
79+
stream=stream,
80+
keepalive_expiry=self._keepalive_expiry,
81+
)
82+
else:
83+
self._connection = AsyncHTTP11Connection(
84+
origin=self._origin,
85+
stream=stream,
86+
keepalive_expiry=self._keepalive_expiry,
87+
)
88+
except Exception as exc:
89+
self._connect_failed = True
90+
raise exc
8691
elif not self._connection.is_available():
8792
raise ConnectionNotAvailable()
8893

@@ -154,27 +159,31 @@ def is_available(self) -> bool:
154159
# If HTTP/2 support is enabled, and the resulting connection could
155160
# end up as HTTP/2 then we should indicate the connection as being
156161
# available to service multiple requests.
157-
return self._http2 and (self._origin.scheme == b"https" or not self._http1)
162+
return (
163+
self._http2
164+
and (self._origin.scheme == b"https" or not self._http1)
165+
and not self._connect_failed
166+
)
158167
return self._connection.is_available()
159168

160169
def has_expired(self) -> bool:
161170
if self._connection is None:
162-
return False
171+
return self._connect_failed
163172
return self._connection.has_expired()
164173

165174
def is_idle(self) -> bool:
166175
if self._connection is None:
167-
return False
176+
return self._connect_failed
168177
return self._connection.is_idle()
169178

170179
def is_closed(self) -> bool:
171180
if self._connection is None:
172-
return False
181+
return self._connect_failed
173182
return self._connection.is_closed()
174183

175184
def info(self) -> str:
176185
if self._connection is None:
177-
return "CONNECTING"
186+
return "CONNECTION FAILED" if self._connect_failed else "CONNECTING"
178187
return self._connection.info()
179188

180189
def __repr__(self) -> str:

Diff for: ‎httpcore/_sync/connection.py

+34-25
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ def __init__(
5252
SyncBackend() if network_backend is None else network_backend
5353
)
5454
self._connection: Optional[ConnectionInterface] = None
55+
self._connect_failed: bool = False
5556
self._request_lock = Lock()
5657

5758
def handle_request(self, request: Request) -> Response:
@@ -62,27 +63,31 @@ def handle_request(self, request: Request) -> Response:
6263

6364
with self._request_lock:
6465
if self._connection is None:
65-
stream = self._connect(request)
66-
67-
ssl_object = stream.get_extra_info("ssl_object")
68-
http2_negotiated = (
69-
ssl_object is not None
70-
and ssl_object.selected_alpn_protocol() == "h2"
71-
)
72-
if http2_negotiated or (self._http2 and not self._http1):
73-
from .http2 import HTTP2Connection
74-
75-
self._connection = HTTP2Connection(
76-
origin=self._origin,
77-
stream=stream,
78-
keepalive_expiry=self._keepalive_expiry,
79-
)
80-
else:
81-
self._connection = HTTP11Connection(
82-
origin=self._origin,
83-
stream=stream,
84-
keepalive_expiry=self._keepalive_expiry,
66+
try:
67+
stream = self._connect(request)
68+
69+
ssl_object = stream.get_extra_info("ssl_object")
70+
http2_negotiated = (
71+
ssl_object is not None
72+
and ssl_object.selected_alpn_protocol() == "h2"
8573
)
74+
if http2_negotiated or (self._http2 and not self._http1):
75+
from .http2 import HTTP2Connection
76+
77+
self._connection = HTTP2Connection(
78+
origin=self._origin,
79+
stream=stream,
80+
keepalive_expiry=self._keepalive_expiry,
81+
)
82+
else:
83+
self._connection = HTTP11Connection(
84+
origin=self._origin,
85+
stream=stream,
86+
keepalive_expiry=self._keepalive_expiry,
87+
)
88+
except Exception as exc:
89+
self._connect_failed = True
90+
raise exc
8691
elif not self._connection.is_available():
8792
raise ConnectionNotAvailable()
8893

@@ -154,27 +159,31 @@ def is_available(self) -> bool:
154159
# If HTTP/2 support is enabled, and the resulting connection could
155160
# end up as HTTP/2 then we should indicate the connection as being
156161
# available to service multiple requests.
157-
return self._http2 and (self._origin.scheme == b"https" or not self._http1)
162+
return (
163+
self._http2
164+
and (self._origin.scheme == b"https" or not self._http1)
165+
and not self._connect_failed
166+
)
158167
return self._connection.is_available()
159168

160169
def has_expired(self) -> bool:
161170
if self._connection is None:
162-
return False
171+
return self._connect_failed
163172
return self._connection.has_expired()
164173

165174
def is_idle(self) -> bool:
166175
if self._connection is None:
167-
return False
176+
return self._connect_failed
168177
return self._connection.is_idle()
169178

170179
def is_closed(self) -> bool:
171180
if self._connection is None:
172-
return False
181+
return self._connect_failed
173182
return self._connection.is_closed()
174183

175184
def info(self) -> str:
176185
if self._connection is None:
177-
return "CONNECTING"
186+
return "CONNECTION FAILED" if self._connect_failed else "CONNECTING"
178187
return self._connection.info()
179188

180189
def __repr__(self) -> str:

Diff for: ‎tests/_async/test_connection_pool.py

+40-4
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import pytest
44
import trio as concurrency
55

6-
from httpcore import AsyncConnectionPool, UnsupportedProtocol
6+
from httpcore import AsyncConnectionPool, ConnectError, UnsupportedProtocol
77
from httpcore.backends.mock import AsyncMockBackend
88

99

@@ -154,10 +154,10 @@ async def trace(name, kwargs):
154154

155155

156156
@pytest.mark.anyio
157-
async def test_connection_pool_with_exception():
157+
async def test_connection_pool_with_http_exception():
158158
"""
159-
HTTP/1.1 requests that result in an exception should not be returned to the
160-
connection pool.
159+
HTTP/1.1 requests that result in an exception during the connection should
160+
not be returned to the connection pool.
161161
"""
162162
network_backend = AsyncMockBackend([b"Wait, this isn't valid HTTP!"])
163163

@@ -192,6 +192,42 @@ async def trace(name, kwargs):
192192
]
193193

194194

195+
@pytest.mark.anyio
196+
async def test_connection_pool_with_connect_exception():
197+
"""
198+
HTTP/1.1 requests that result in an exception during connection should not
199+
be returned to the connection pool.
200+
"""
201+
202+
class FailedConnectBackend(AsyncMockBackend):
203+
async def connect_tcp(
204+
self, host: str, port: int, timeout: float = None, local_address: str = None
205+
):
206+
raise ConnectError("Could not connect")
207+
208+
network_backend = FailedConnectBackend([])
209+
210+
called = []
211+
212+
async def trace(name, kwargs):
213+
called.append(name)
214+
215+
async with AsyncConnectionPool(network_backend=network_backend) as pool:
216+
# Sending an initial request, which once complete will not return to the pool.
217+
with pytest.raises(Exception):
218+
await pool.request(
219+
"GET", "https://example.com/", extensions={"trace": trace}
220+
)
221+
222+
info = [repr(c) for c in pool.connections]
223+
assert info == []
224+
225+
assert called == [
226+
"connection.connect_tcp.started",
227+
"connection.connect_tcp.failed",
228+
]
229+
230+
195231
@pytest.mark.anyio
196232
async def test_connection_pool_with_immediate_expiry():
197233
"""

Diff for: ‎tests/_sync/test_connection_pool.py

+40-4
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import pytest
44
from tests import concurrency
55

6-
from httpcore import ConnectionPool, UnsupportedProtocol
6+
from httpcore import ConnectionPool, ConnectError, UnsupportedProtocol
77
from httpcore.backends.mock import MockBackend
88

99

@@ -154,10 +154,10 @@ def trace(name, kwargs):
154154

155155

156156

157-
def test_connection_pool_with_exception():
157+
def test_connection_pool_with_http_exception():
158158
"""
159-
HTTP/1.1 requests that result in an exception should not be returned to the
160-
connection pool.
159+
HTTP/1.1 requests that result in an exception during the connection should
160+
not be returned to the connection pool.
161161
"""
162162
network_backend = MockBackend([b"Wait, this isn't valid HTTP!"])
163163

@@ -193,6 +193,42 @@ def trace(name, kwargs):
193193

194194

195195

196+
def test_connection_pool_with_connect_exception():
197+
"""
198+
HTTP/1.1 requests that result in an exception during connection should not
199+
be returned to the connection pool.
200+
"""
201+
202+
class FailedConnectBackend(MockBackend):
203+
def connect_tcp(
204+
self, host: str, port: int, timeout: float = None, local_address: str = None
205+
):
206+
raise ConnectError("Could not connect")
207+
208+
network_backend = FailedConnectBackend([])
209+
210+
called = []
211+
212+
def trace(name, kwargs):
213+
called.append(name)
214+
215+
with ConnectionPool(network_backend=network_backend) as pool:
216+
# Sending an initial request, which once complete will not return to the pool.
217+
with pytest.raises(Exception):
218+
pool.request(
219+
"GET", "https://example.com/", extensions={"trace": trace}
220+
)
221+
222+
info = [repr(c) for c in pool.connections]
223+
assert info == []
224+
225+
assert called == [
226+
"connection.connect_tcp.started",
227+
"connection.connect_tcp.failed",
228+
]
229+
230+
231+
196232
def test_connection_pool_with_immediate_expiry():
197233
"""
198234
Connection pools with keepalive_expiry=0.0 should immediately expire

0 commit comments

Comments
 (0)