Skip to content

Commit c3db70d

Browse files
MarkusSintonenMarkusSintonen
committed
Move connection poll check away from pool expiry checks
1 parent da86ca4 commit c3db70d

File tree

9 files changed

+244
-28
lines changed

9 files changed

+244
-28
lines changed

‎httpcore/_async/connection_pool.py‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,11 @@
55

66
from .._backends.auto import AutoBackend
77
from .._backends.base import SOCKET_OPTION, AsyncNetworkBackend
8-
from .._exceptions import ConnectionNotAvailable, UnsupportedProtocol
8+
from .._exceptions import (
9+
ConnectionNotAvailable,
10+
ServerDisconnectedInternalError,
11+
UnsupportedProtocol,
12+
)
913
from .._models import Origin, Request, Response
1014
from .._synchronization import AsyncEvent, AsyncShieldCancellation, AsyncThreadLock
1115
from .connection import AsyncHTTPConnection
@@ -196,7 +200,7 @@ async def handle_async_request(self, request: Request) -> Response:
196200
response = await connection.handle_async_request(
197201
pool_request.request
198202
)
199-
except ConnectionNotAvailable:
203+
except (ConnectionNotAvailable, ServerDisconnectedInternalError):
200204
# In some cases a connection may initially be available to
201205
# handle a request, but then become unavailable.
202206
#

‎httpcore/_async/http11.py‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
ConnectionNotAvailable,
2222
LocalProtocolError,
2323
RemoteProtocolError,
24+
ServerDisconnectedInternalError,
2425
WriteError,
2526
map_exceptions,
2627
)
@@ -59,9 +60,10 @@ def __init__(
5960
) -> None:
6061
self._origin = origin
6162
self._network_stream = stream
62-
self._keepalive_expiry: Optional[float] = keepalive_expiry
63+
self._keepalive_expiry = keepalive_expiry
6364
self._expire_at: Optional[float] = None
6465
self._state = HTTPConnectionState.NEW
66+
self._server_disconnected = False
6567
self._state_lock = AsyncLock()
6668
self._request_count = 0
6769
self._h11_state = h11.Connection(
@@ -77,6 +79,17 @@ async def handle_async_request(self, request: Request) -> Response:
7779
)
7880

7981
async with self._state_lock:
82+
# If the HTTP connection is idle but the socket is readable, then the
83+
# only valid state is that the socket is about to return b"", indicating
84+
# a server-initiated disconnect.
85+
server_disconnected = (
86+
self._state == HTTPConnectionState.IDLE
87+
and self._network_stream.get_extra_info("is_readable")
88+
)
89+
if server_disconnected or self._server_disconnected:
90+
self._server_disconnected = True
91+
raise ServerDisconnectedInternalError()
92+
8093
if self._state in (HTTPConnectionState.NEW, HTTPConnectionState.IDLE):
8194
self._request_count += 1
8295
self._state = HTTPConnectionState.ACTIVE
@@ -279,18 +292,11 @@ def is_available(self) -> bool:
279292
return self._state == HTTPConnectionState.IDLE
280293

281294
def has_expired(self) -> bool:
282-
now = time.monotonic()
283-
keepalive_expired = self._expire_at is not None and now > self._expire_at
284-
285-
# If the HTTP connection is idle but the socket is readable, then the
286-
# only valid state is that the socket is about to return b"", indicating
287-
# a server-initiated disconnect.
288-
server_disconnected = (
289-
self._state == HTTPConnectionState.IDLE
290-
and self._network_stream.get_extra_info("is_readable")
291-
)
295+
if self._server_disconnected:
296+
return True
292297

293-
return keepalive_expired or server_disconnected
298+
now = time.monotonic()
299+
return self._expire_at is not None and now > self._expire_at
294300

295301
def is_idle(self) -> bool:
296302
return self._state == HTTPConnectionState.IDLE

‎httpcore/_exceptions.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,10 @@ class ConnectionNotAvailable(Exception):
1919
pass
2020

2121

22+
class ServerDisconnectedInternalError(Exception):
23+
pass
24+
25+
2226
class ProxyError(Exception):
2327
pass
2428

‎httpcore/_sync/connection_pool.py‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,11 @@
55

66
from .._backends.sync import SyncBackend
77
from .._backends.base import SOCKET_OPTION, NetworkBackend
8-
from .._exceptions import ConnectionNotAvailable, UnsupportedProtocol
8+
from .._exceptions import (
9+
ConnectionNotAvailable,
10+
ServerDisconnectedInternalError,
11+
UnsupportedProtocol,
12+
)
913
from .._models import Origin, Request, Response
1014
from .._synchronization import Event, ShieldCancellation, ThreadLock
1115
from .connection import HTTPConnection
@@ -196,7 +200,7 @@ def handle_request(self, request: Request) -> Response:
196200
response = connection.handle_request(
197201
pool_request.request
198202
)
199-
except ConnectionNotAvailable:
203+
except (ConnectionNotAvailable, ServerDisconnectedInternalError):
200204
# In some cases a connection may initially be available to
201205
# handle a request, but then become unavailable.
202206
#

‎httpcore/_sync/http11.py‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
ConnectionNotAvailable,
2222
LocalProtocolError,
2323
RemoteProtocolError,
24+
ServerDisconnectedInternalError,
2425
WriteError,
2526
map_exceptions,
2627
)
@@ -59,9 +60,10 @@ def __init__(
5960
) -> None:
6061
self._origin = origin
6162
self._network_stream = stream
62-
self._keepalive_expiry: Optional[float] = keepalive_expiry
63+
self._keepalive_expiry = keepalive_expiry
6364
self._expire_at: Optional[float] = None
6465
self._state = HTTPConnectionState.NEW
66+
self._server_disconnected = False
6567
self._state_lock = Lock()
6668
self._request_count = 0
6769
self._h11_state = h11.Connection(
@@ -77,6 +79,17 @@ def handle_request(self, request: Request) -> Response:
7779
)
7880

7981
with self._state_lock:
82+
# If the HTTP connection is idle but the socket is readable, then the
83+
# only valid state is that the socket is about to return b"", indicating
84+
# a server-initiated disconnect.
85+
server_disconnected = (
86+
self._state == HTTPConnectionState.IDLE
87+
and self._network_stream.get_extra_info("is_readable")
88+
)
89+
if server_disconnected or self._server_disconnected:
90+
self._server_disconnected = True
91+
raise ServerDisconnectedInternalError()
92+
8093
if self._state in (HTTPConnectionState.NEW, HTTPConnectionState.IDLE):
8194
self._request_count += 1
8295
self._state = HTTPConnectionState.ACTIVE
@@ -279,18 +292,11 @@ def is_available(self) -> bool:
279292
return self._state == HTTPConnectionState.IDLE
280293

281294
def has_expired(self) -> bool:
282-
now = time.monotonic()
283-
keepalive_expired = self._expire_at is not None and now > self._expire_at
284-
285-
# If the HTTP connection is idle but the socket is readable, then the
286-
# only valid state is that the socket is about to return b"", indicating
287-
# a server-initiated disconnect.
288-
server_disconnected = (
289-
self._state == HTTPConnectionState.IDLE
290-
and self._network_stream.get_extra_info("is_readable")
291-
)
295+
if self._server_disconnected:
296+
return True
292297

293-
return keepalive_expired or server_disconnected
298+
now = time.monotonic()
299+
return self._expire_at is not None and now > self._expire_at
294300

295301
def is_idle(self) -> bool:
296302
return self._state == HTTPConnectionState.IDLE

‎tests/_async/test_connection_pool.py‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -687,6 +687,63 @@ async def test_connection_pool_closed_while_request_in_flight():
687687
await response.aread()
688688

689689

690+
@pytest.mark.anyio
691+
async def test_connection_pool_with_idle_broken_connection():
692+
"""
693+
Pool gives a new connection when an idle connection gets readable (ie broken) while in the pool.
694+
"""
695+
696+
class MockStream(httpcore.AsyncMockStream):
697+
def __init__(self, buffer: typing.List[bytes]):
698+
super().__init__(buffer)
699+
self.mock_is_readable = False
700+
701+
def get_extra_info(self, info: str) -> typing.Any:
702+
if info == "is_readable":
703+
return self.mock_is_readable
704+
return super().get_extra_info(info) # pragma: nocover
705+
706+
class MockBackend(httpcore.AsyncMockBackend):
707+
async def connect_tcp(
708+
self, *args: typing.Any, **kwargs: typing.Any
709+
) -> MockStream:
710+
return MockStream(list(self._buffer))
711+
712+
network_backend = MockBackend(
713+
[
714+
b"HTTP/1.1 200 OK\r\n",
715+
b"Content-Type: plain/text\r\n",
716+
b"Content-Length: 13\r\n",
717+
b"\r\n",
718+
b"Hello, world!",
719+
b"HTTP/1.1 200 OK\r\n",
720+
b"Content-Type: plain/text\r\n",
721+
b"Content-Length: 13\r\n",
722+
b"\r\n",
723+
b"Hello, world!",
724+
]
725+
)
726+
async with httpcore.AsyncConnectionPool(
727+
network_backend=network_backend, max_connections=1
728+
) as pool:
729+
await pool.request("GET", "https://example.com/")
730+
assert len(pool.connections) == 1
731+
conn = pool.connections[0]
732+
await pool.request("GET", "https://example.com/")
733+
assert len(pool.connections) == 1
734+
assert conn is pool.connections[0], "Should reuse connection"
735+
736+
# Simulate network breakage
737+
assert conn.is_idle()
738+
conn._connection._network_stream.mock_is_readable = True # type: ignore[attr-defined]
739+
740+
await pool.request("GET", "https://example.com/")
741+
assert len(pool.connections) == 1
742+
new_conn = pool.connections[0]
743+
assert new_conn is not conn, "Should be a new connection"
744+
assert not new_conn._connection._network_stream.mock_is_readable # type: ignore[attr-defined]
745+
746+
690747
@pytest.mark.anyio
691748
async def test_connection_pool_timeout():
692749
"""

‎tests/_async/test_http11.py‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
1+
import typing
2+
13
import pytest
24

35
import httpcore
6+
from httpcore._exceptions import ServerDisconnectedInternalError
47

58

69
@pytest.mark.anyio
@@ -167,6 +170,42 @@ async def test_http11_connection_handles_one_active_request():
167170
await conn.request("GET", "https://example.com/")
168171

169172

173+
@pytest.mark.anyio
174+
async def test_http11_idle_connection_checks_readable_state():
175+
"""
176+
Idle connection can not be readable when requesting.
177+
"""
178+
179+
class MockStream(httpcore.AsyncMockStream):
180+
def __init__(self, buffer: typing.List[bytes]):
181+
super().__init__(buffer)
182+
self.mock_is_readable = False
183+
184+
def get_extra_info(self, info: str) -> typing.Any:
185+
if info == "is_readable":
186+
return self.mock_is_readable
187+
return super().get_extra_info(info) # pragma: nocover
188+
189+
origin = httpcore.Origin(b"https", b"example.com", 443)
190+
stream = MockStream(
191+
[
192+
b"HTTP/1.1 200 OK\r\n",
193+
b"Content-Type: plain/text\r\n",
194+
b"Content-Length: 13\r\n",
195+
b"\r\n",
196+
b"Hello, world!",
197+
]
198+
)
199+
async with httpcore.AsyncHTTP11Connection(origin=origin, stream=stream) as conn:
200+
await conn.request("GET", "https://example.com/")
201+
202+
assert conn.is_idle()
203+
stream.mock_is_readable = True # Simulate connection breakage
204+
205+
with pytest.raises(ServerDisconnectedInternalError):
206+
await conn.request("GET", "https://example.com/")
207+
208+
170209
@pytest.mark.anyio
171210
async def test_http11_connection_attempt_close():
172211
"""

‎tests/_sync/test_connection_pool.py‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -688,6 +688,63 @@ def test_connection_pool_closed_while_request_in_flight():
688688

689689

690690

691+
def test_connection_pool_with_idle_broken_connection():
692+
"""
693+
Pool gives a new connection when an idle connection gets readable (ie broken) while in the pool.
694+
"""
695+
696+
class MockStream(httpcore.MockStream):
697+
def __init__(self, buffer: typing.List[bytes]):
698+
super().__init__(buffer)
699+
self.mock_is_readable = False
700+
701+
def get_extra_info(self, info: str) -> typing.Any:
702+
if info == "is_readable":
703+
return self.mock_is_readable
704+
return super().get_extra_info(info) # pragma: nocover
705+
706+
class MockBackend(httpcore.MockBackend):
707+
def connect_tcp(
708+
self, *args: typing.Any, **kwargs: typing.Any
709+
) -> MockStream:
710+
return MockStream(list(self._buffer))
711+
712+
network_backend = MockBackend(
713+
[
714+
b"HTTP/1.1 200 OK\r\n",
715+
b"Content-Type: plain/text\r\n",
716+
b"Content-Length: 13\r\n",
717+
b"\r\n",
718+
b"Hello, world!",
719+
b"HTTP/1.1 200 OK\r\n",
720+
b"Content-Type: plain/text\r\n",
721+
b"Content-Length: 13\r\n",
722+
b"\r\n",
723+
b"Hello, world!",
724+
]
725+
)
726+
with httpcore.ConnectionPool(
727+
network_backend=network_backend, max_connections=1
728+
) as pool:
729+
pool.request("GET", "https://example.com/")
730+
assert len(pool.connections) == 1
731+
conn = pool.connections[0]
732+
pool.request("GET", "https://example.com/")
733+
assert len(pool.connections) == 1
734+
assert conn is pool.connections[0], "Should reuse connection"
735+
736+
# Simulate network breakage
737+
assert conn.is_idle()
738+
conn._connection._network_stream.mock_is_readable = True # type: ignore[attr-defined]
739+
740+
pool.request("GET", "https://example.com/")
741+
assert len(pool.connections) == 1
742+
new_conn = pool.connections[0]
743+
assert new_conn is not conn, "Should be a new connection"
744+
assert not new_conn._connection._network_stream.mock_is_readable # type: ignore[attr-defined]
745+
746+
747+
691748
def test_connection_pool_timeout():
692749
"""
693750
Ensure that exceeding max_connections can cause a request to timeout.

0 commit comments

Comments
 (0)