Fix max_keepalive_connections

This commit fixes a bug in both sync and async connection pools
where idle connections were dropped from the pool even when
max_keepalive_connections limit has not been reached. This
happened because the check compared this number to the total
number of connections, not only the idle ones.

Author: blazewicz

CHANGELOG.md

@@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
+## Unreleased
+
+- Fix `max_keepalive_connections` not being properly handled. (#1000)
+
 ## Version 1.0.7 (November 15th, 2024)
 
 - Support `proxy=…` configuration on `ConnectionPool()`. (#974)

httpcore/_async/connection_pool.py

@@ -291,7 +291,7 @@ def _assign_requests_to_connections(self) -> list[AsyncConnectionInterface]:
                 closing_connections.append(connection)
             elif (
                 connection.is_idle()
-                and len([connection.is_idle() for connection in self._connections])
+                and sum(connection.is_idle() for connection in self._connections)
                 > self._max_keepalive_connections
             ):
                 # log: "closing idle connection"

httpcore/_sync/connection_pool.py

@@ -291,7 +291,7 @@ def _assign_requests_to_connections(self) -> list[ConnectionInterface]:
                 closing_connections.append(connection)
             elif (
                 connection.is_idle()
-                and len([connection.is_idle() for connection in self._connections])
+                and sum(connection.is_idle() for connection in self._connections)
                 > self._max_keepalive_connections
             ):
                 # log: "closing idle connection"

tests/_async/test_connection_pool.py

@@ -30,7 +30,7 @@ async def test_connection_pool_with_keepalive():
     )
 
     async with httpcore.AsyncConnectionPool(
-        network_backend=network_backend,
+        network_backend=network_backend, max_keepalive_connections=1
     ) as pool:
         # Sending an intial request, which once complete will return to the pool, IDLE.
         async with pool.stream("GET", "https://example.com/") as response:
@@ -79,28 +79,33 @@ async def test_connection_pool_with_keepalive():
         )
 
         # Sending a request to a different origin will not reuse the existing IDLE connection.
-        async with pool.stream("GET", "http://example.com/") as response:
+        async with pool.stream("GET", "http://example.com/") as response_1, pool.stream(
+            "GET", "http://example.com/"
+        ) as response_2:
             info = [repr(c) for c in pool.connections]
             assert info == [
                 "<AsyncHTTPConnection ['https://example.com:443', HTTP/1.1, IDLE, Request Count: 2]>",
                 "<AsyncHTTPConnection ['http://example.com:80', HTTP/1.1, ACTIVE, Request Count: 1]>",
+                "<AsyncHTTPConnection ['http://example.com:80', HTTP/1.1, ACTIVE, Request Count: 1]>",
             ]
             assert (
                 repr(pool)
-                == "<AsyncConnectionPool [Requests: 1 active, 0 queued | Connections: 1 active, 1 idle]>"
+                == "<AsyncConnectionPool [Requests: 2 active, 0 queued | Connections: 2 active, 1 idle]>"
             )
-            await response.aread()
+            await response_1.aread()
+            await response_2.aread()
 
-        assert response.status == 200
-        assert response.content == b"Hello, world!"
+        assert response_1.status == 200
+        assert response_1.content == b"Hello, world!"
+        assert response_2.status == 200
+        assert response_2.content == b"Hello, world!"
         info = [repr(c) for c in pool.connections]
         assert info == [
-            "<AsyncHTTPConnection ['https://example.com:443', HTTP/1.1, IDLE, Request Count: 2]>",
             "<AsyncHTTPConnection ['http://example.com:80', HTTP/1.1, IDLE, Request Count: 1]>",
         ]
         assert (
             repr(pool)
-            == "<AsyncConnectionPool [Requests: 0 active, 0 queued | Connections: 0 active, 2 idle]>"
+            == "<AsyncConnectionPool [Requests: 0 active, 0 queued | Connections: 0 active, 1 idle]>"
         )
 
 

tests/_sync/test_connection_pool.py

@@ -30,7 +30,7 @@ def test_connection_pool_with_keepalive():
     )
 
     with httpcore.ConnectionPool(
-        network_backend=network_backend,
+        network_backend=network_backend, max_keepalive_connections=1
     ) as pool:
         # Sending an intial request, which once complete will return to the pool, IDLE.
         with pool.stream("GET", "https://example.com/") as response:
@@ -79,28 +79,33 @@ def test_connection_pool_with_keepalive():
         )
 
         # Sending a request to a different origin will not reuse the existing IDLE connection.
-        with pool.stream("GET", "http://example.com/") as response:
+        with pool.stream("GET", "http://example.com/") as response_1, pool.stream(
+            "GET", "http://example.com/"
+        ) as response_2:
             info = [repr(c) for c in pool.connections]
             assert info == [
                 "<HTTPConnection ['https://example.com:443', HTTP/1.1, IDLE, Request Count: 2]>",
                 "<HTTPConnection ['http://example.com:80', HTTP/1.1, ACTIVE, Request Count: 1]>",
+                "<HTTPConnection ['http://example.com:80', HTTP/1.1, ACTIVE, Request Count: 1]>",
             ]
             assert (
                 repr(pool)
-                == "<ConnectionPool [Requests: 1 active, 0 queued | Connections: 1 active, 1 idle]>"
+                == "<ConnectionPool [Requests: 2 active, 0 queued | Connections: 2 active, 1 idle]>"
             )
-            response.read()
+            response_1.read()
+            response_2.read()
 
-        assert response.status == 200
-        assert response.content == b"Hello, world!"
+        assert response_1.status == 200
+        assert response_1.content == b"Hello, world!"
+        assert response_2.status == 200
+        assert response_2.content == b"Hello, world!"
         info = [repr(c) for c in pool.connections]
         assert info == [
-            "<HTTPConnection ['https://example.com:443', HTTP/1.1, IDLE, Request Count: 2]>",
             "<HTTPConnection ['http://example.com:80', HTTP/1.1, IDLE, Request Count: 1]>",
         ]
         assert (
             repr(pool)
-            == "<ConnectionPool [Requests: 0 active, 0 queued | Connections: 0 active, 2 idle]>"
+            == "<ConnectionPool [Requests: 0 active, 0 queued | Connections: 0 active, 1 idle]>"
         )