Skip to content

Commit 27e73bb

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

File tree

10 files changed

+304
-28
lines changed

10 files changed

+304
-28
lines changed

‎httpcore/__init__.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
ReadError,
3030
ReadTimeout,
3131
RemoteProtocolError,
32+
ServerDisconnectedError,
3233
TimeoutException,
3334
UnsupportedProtocol,
3435
WriteError,
@@ -114,6 +115,7 @@ def __init__(self, *args, **kwargs): # type: ignore
114115
"SOCKET_OPTION",
115116
# exceptions
116117
"ConnectionNotAvailable",
118+
"ServerDisconnectedError",
117119
"ProxyError",
118120
"ProtocolError",
119121
"LocalProtocolError",

‎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+
ServerDisconnectedError,
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, ServerDisconnectedError):
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: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
ConnectionNotAvailable,
2222
LocalProtocolError,
2323
RemoteProtocolError,
24+
ServerDisconnectedError,
2425
WriteError,
2526
map_exceptions,
2627
)
@@ -45,6 +46,7 @@ class HTTPConnectionState(enum.IntEnum):
4546
ACTIVE = 1
4647
IDLE = 2
4748
CLOSED = 3
49+
SERVER_DISCONNECTED = 4
4850

4951

5052
class AsyncHTTP11Connection(AsyncConnectionInterface):
@@ -59,7 +61,7 @@ def __init__(
5961
) -> None:
6062
self._origin = origin
6163
self._network_stream = stream
62-
self._keepalive_expiry: Optional[float] = keepalive_expiry
64+
self._keepalive_expiry = keepalive_expiry
6365
self._expire_at: Optional[float] = None
6466
self._state = HTTPConnectionState.NEW
6567
self._state_lock = AsyncLock()
@@ -77,6 +79,20 @@ async def handle_async_request(self, request: Request) -> Response:
7779
)
7880

7981
async with self._state_lock:
82+
if self._state == HTTPConnectionState.SERVER_DISCONNECTED:
83+
raise ServerDisconnectedError()
84+
85+
# If the HTTP connection is idle but the socket is readable, then the
86+
# only valid state is that the socket is about to return b"", indicating
87+
# a server-initiated disconnect.
88+
server_disconnected = (
89+
self._state == HTTPConnectionState.IDLE
90+
and self._network_stream.get_extra_info("is_readable")
91+
)
92+
if server_disconnected:
93+
self._state = HTTPConnectionState.SERVER_DISCONNECTED
94+
raise ServerDisconnectedError()
95+
8096
if self._state in (HTTPConnectionState.NEW, HTTPConnectionState.IDLE):
8197
self._request_count += 1
8298
self._state = HTTPConnectionState.ACTIVE
@@ -279,18 +295,13 @@ def is_available(self) -> bool:
279295
return self._state == HTTPConnectionState.IDLE
280296

281297
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-
)
298+
if self._state == HTTPConnectionState.SERVER_DISCONNECTED:
299+
# Connection that is disconnected by the server is considered expired.
300+
# Pool then cleans up this connection by closing it.
301+
return True
292302

293-
return keepalive_expired or server_disconnected
303+
now = time.monotonic()
304+
return self._expire_at is not None and now > self._expire_at
294305

295306
def is_idle(self) -> bool:
296307
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 ServerDisconnectedError(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+
ServerDisconnectedError,
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, ServerDisconnectedError):
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: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
ConnectionNotAvailable,
2222
LocalProtocolError,
2323
RemoteProtocolError,
24+
ServerDisconnectedError,
2425
WriteError,
2526
map_exceptions,
2627
)
@@ -45,6 +46,7 @@ class HTTPConnectionState(enum.IntEnum):
4546
ACTIVE = 1
4647
IDLE = 2
4748
CLOSED = 3
49+
SERVER_DISCONNECTED = 4
4850

4951

5052
class HTTP11Connection(ConnectionInterface):
@@ -59,7 +61,7 @@ def __init__(
5961
) -> None:
6062
self._origin = origin
6163
self._network_stream = stream
62-
self._keepalive_expiry: Optional[float] = keepalive_expiry
64+
self._keepalive_expiry = keepalive_expiry
6365
self._expire_at: Optional[float] = None
6466
self._state = HTTPConnectionState.NEW
6567
self._state_lock = Lock()
@@ -77,6 +79,20 @@ def handle_request(self, request: Request) -> Response:
7779
)
7880

7981
with self._state_lock:
82+
if self._state == HTTPConnectionState.SERVER_DISCONNECTED:
83+
raise ServerDisconnectedError()
84+
85+
# If the HTTP connection is idle but the socket is readable, then the
86+
# only valid state is that the socket is about to return b"", indicating
87+
# a server-initiated disconnect.
88+
server_disconnected = (
89+
self._state == HTTPConnectionState.IDLE
90+
and self._network_stream.get_extra_info("is_readable")
91+
)
92+
if server_disconnected:
93+
self._state = HTTPConnectionState.SERVER_DISCONNECTED
94+
raise ServerDisconnectedError()
95+
8096
if self._state in (HTTPConnectionState.NEW, HTTPConnectionState.IDLE):
8197
self._request_count += 1
8298
self._state = HTTPConnectionState.ACTIVE
@@ -279,18 +295,13 @@ def is_available(self) -> bool:
279295
return self._state == HTTPConnectionState.IDLE
280296

281297
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-
)
298+
if self._state == HTTPConnectionState.SERVER_DISCONNECTED:
299+
# Connection that is disconnected by the server is considered expired.
300+
# Pool then cleans up this connection by closing it.
301+
return True
292302

293-
return keepalive_expired or server_disconnected
303+
now = time.monotonic()
304+
return self._expire_at is not None and now > self._expire_at
294305

295306
def is_idle(self) -> bool:
296307
return self._state == HTTPConnectionState.IDLE

‎tests/_async/test_connection_pool.py‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -687,6 +687,82 @@ 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+
streams = [
707+
MockStream(
708+
[
709+
b"HTTP/1.1 200 OK\r\n",
710+
b"Content-Type: plain/text\r\n",
711+
b"Content-Length: 15\r\n",
712+
b"\r\n",
713+
b"Hello, world 1!",
714+
b"HTTP/1.1 200 OK\r\n",
715+
b"Content-Type: plain/text\r\n",
716+
b"Content-Length: 15\r\n",
717+
b"\r\n",
718+
b"Hello, world 2!",
719+
]
720+
),
721+
MockStream(
722+
[
723+
b"HTTP/1.1 200 OK\r\n",
724+
b"Content-Type: plain/text\r\n",
725+
b"Content-Length: 29\r\n",
726+
b"\r\n",
727+
b"Hello, world from new stream!",
728+
]
729+
),
730+
]
731+
732+
class MockBackend(httpcore.AsyncMockBackend):
733+
async def connect_tcp(
734+
self, *args: typing.Any, **kwargs: typing.Any
735+
) -> MockStream:
736+
return streams.pop(0)
737+
738+
async with httpcore.AsyncConnectionPool(
739+
network_backend=MockBackend([]), max_connections=1
740+
) as pool:
741+
res = await pool.request("GET", "https://example.com/")
742+
assert (await res.aread()) == b"Hello, world 1!"
743+
744+
assert len(pool.connections) == 1
745+
conn = pool.connections[0]
746+
747+
res = await pool.request("GET", "https://example.com/")
748+
assert (await res.aread()) == b"Hello, world 2!"
749+
750+
assert len(pool.connections) == 1
751+
assert conn is pool.connections[0], "Should reuse connection"
752+
753+
# Simulate network breakage
754+
assert conn.is_idle()
755+
conn._connection._network_stream.mock_is_readable = True # type: ignore[attr-defined]
756+
757+
res = await pool.request("GET", "https://example.com/")
758+
assert (await res.aread()) == b"Hello, world from new stream!"
759+
760+
assert len(pool.connections) == 1
761+
new_conn = pool.connections[0]
762+
assert new_conn is not conn, "Should be a new connection"
763+
assert not new_conn._connection._network_stream.mock_is_readable # type: ignore[attr-defined]
764+
765+
690766
@pytest.mark.anyio
691767
async def test_connection_pool_timeout():
692768
"""

‎tests/_async/test_http11.py‎

Lines changed: 44 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 ServerDisconnectedError
47

58

69
@pytest.mark.anyio
@@ -167,6 +170,47 @@ 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() and not conn.has_expired()
203+
stream.mock_is_readable = True # Simulate connection breakage
204+
205+
with pytest.raises(ServerDisconnectedError):
206+
await conn.request("GET", "https://example.com/")
207+
assert conn.has_expired() and not conn.is_idle()
208+
209+
with pytest.raises(ServerDisconnectedError):
210+
await conn.request("GET", "https://example.com/")
211+
assert conn.has_expired() and not conn.is_idle()
212+
213+
170214
@pytest.mark.anyio
171215
async def test_http11_connection_attempt_close():
172216
"""

0 commit comments

Comments
 (0)