Make enable_cleanup_closed a NOOP for Python 3.12.7+ and 3.13.1+ (#9726)

(cherry picked from commit c3a9c3e)

Author: bdraco

CHANGES/9726.misc.rst

@@ -0,0 +1 @@
+Passing ``enable_cleanup_closed`` to :py:class:`aiohttp.TCPConnector` is now ignored on Python 3.12.7+ and 3.13.1+ since the underlying bug that caused asyncio to leak SSL connections has been fixed upstream -- by :user:`bdraco`.

aiohttp/connector.py

@@ -76,6 +76,14 @@
 HTTP_AND_EMPTY_SCHEMA_SET = HTTP_SCHEMA_SET | EMPTY_SCHEMA_SET
 HIGH_LEVEL_SCHEMA_SET = HTTP_AND_EMPTY_SCHEMA_SET | WS_SCHEMA_SET
 
+NEEDS_CLEANUP_CLOSED = (3, 13, 0) <= sys.version_info < (
+    3,
+    13,
+    1,
+) or sys.version_info < (3, 12, 7)
+# Cleanup closed is no longer needed after https://github.com/python/cpython/pull/118960
+# which first appeared in Python 3.12.7 and 3.13.1
+
 
 __all__ = ("BaseConnector", "TCPConnector", "UnixConnector", "NamedPipeConnector")
 
@@ -279,6 +287,17 @@ def __init__(
 
         # start cleanup closed transports task
         self._cleanup_closed_handle: Optional[asyncio.TimerHandle] = None
+
+        if enable_cleanup_closed and not NEEDS_CLEANUP_CLOSED:
+            warnings.warn(
+                "enable_cleanup_closed ignored because "
+                "https://github.com/python/cpython/pull/118960 is fixed in "
+                f"in Python version {sys.version_info}",
+                DeprecationWarning,
+                stacklevel=2,
+            )
+            enable_cleanup_closed = False
+
         self._cleanup_closed_disabled = not enable_cleanup_closed
         self._cleanup_closed_transports: List[Optional[asyncio.Transport]] = []
         self._cleanup_closed()

docs/client_reference.rst

@@ -967,10 +967,14 @@ is controlled by *force_close* constructor's parameter).
                             connection releasing (optional).
 
    :param bool enable_cleanup_closed: some SSL servers do not properly complete
-      SSL shutdown process, in that case asyncio leaks ssl connections.
+      SSL shutdown process, in that case asyncio leaks SSL connections.
       If this parameter is set to True, aiohttp additionally aborts underlining
       transport after 2 seconds. It is off by default.
 
+      For Python version 3.12.7+, or 3.13.1 and later,
+      this parameter is ignored because the asyncio SSL connection
+      leak is fixed in these versions of Python.
+
 
    :param loop: :ref:`event loop<asyncio-event-loop>`
       used for handling connections.

tests/conftest.py

@@ -7,7 +7,7 @@
 from hashlib import md5, sha1, sha256
 from pathlib import Path
 from tempfile import TemporaryDirectory
-from typing import Any
+from typing import Any, Generator
 from unittest import mock
 from uuid import uuid4
 
@@ -238,3 +238,14 @@ def key(key_data: Any):
 @pytest.fixture
 def ws_key(key: Any):
     return base64.b64encode(sha1(key + WS_KEY).digest()).decode()
+
+
+@pytest.fixture
+def enable_cleanup_closed() -> Generator[None, None, None]:
+    """Fixture to override the NEEDS_CLEANUP_CLOSED flag.
+
+    On Python 3.12.7+ and 3.13.1+ enable_cleanup_closed is not needed,
+    however we still want to test that it works.
+    """
+    with mock.patch("aiohttp.connector.NEEDS_CLEANUP_CLOSED", True):
+        yield

tests/test_connector.py

@@ -14,6 +14,7 @@
 
 import pytest
 from aiohappyeyeballs import AddrInfoType
+from pytest_mock import MockerFixture
 from yarl import URL
 
 import aiohttp
@@ -359,6 +360,7 @@ async def test_get_expired(loop: asyncio.AbstractEventLoop) -> None:
     await conn.close()
 
 
+@pytest.mark.usefixtures("enable_cleanup_closed")
 async def test_get_expired_ssl(loop: asyncio.AbstractEventLoop) -> None:
     conn = aiohttp.BaseConnector(enable_cleanup_closed=True)
     key = ConnectionKey("localhost", 80, True, False, None, None, None)
@@ -425,9 +427,16 @@ async def test_release(loop, key) -> None:
     await conn.close()
 
 
-async def test_release_ssl_transport(loop, ssl_key) -> None:
-    conn = aiohttp.BaseConnector(loop=loop, enable_cleanup_closed=True)
-    conn._release_waiter = mock.Mock()
+@pytest.mark.usefixtures("enable_cleanup_closed")
+async def test_release_ssl_transport(
+    loop: asyncio.AbstractEventLoop, ssl_key: ConnectionKey
+) -> None:
+    conn = aiohttp.BaseConnector(enable_cleanup_closed=True)
+    with mock.patch.object(conn, "_release_waiter", autospec=True, spec_set=True):
+        proto = create_mocked_conn(loop)
+        transport = proto.transport
+        conn._acquired.add(proto)
+        conn._acquired_per_host[ssl_key].add(proto)
 
     proto = mock.Mock()
     transport = proto.transport
@@ -1682,6 +1691,7 @@ async def test_close_during_connect(loop: asyncio.AbstractEventLoop) -> None:
     assert proto.close.called
 
 
+@pytest.mark.usefixtures("enable_cleanup_closed")
 async def test_ctor_cleanup() -> None:
     loop = mock.Mock()
     loop.time.return_value = 1.5
@@ -1712,8 +1722,11 @@ async def test_cleanup(key) -> None:
     assert conn._cleanup_handle is None
 
 
-async def test_cleanup_close_ssl_transport(ssl_key) -> None:
-    proto = mock.Mock()
+@pytest.mark.usefixtures("enable_cleanup_closed")
+async def test_cleanup_close_ssl_transport(
+    loop: asyncio.AbstractEventLoop, ssl_key: ConnectionKey
+) -> None:
+    proto = create_mocked_conn(loop)
     transport = proto.transport
     testset = {ssl_key: [(proto, 10)]}
 
@@ -1769,7 +1782,10 @@ async def test_cleanup3(key) -> None:
     await conn.close()
 
 
-async def test_cleanup_closed(loop, mocker) -> None:
+@pytest.mark.usefixtures("enable_cleanup_closed")
+async def test_cleanup_closed(
+    loop: asyncio.AbstractEventLoop, mocker: MockerFixture
+) -> None:
     if not hasattr(loop, "__dict__"):
         pytest.skip("can not override loop attributes")
 
@@ -1786,8 +1802,19 @@ async def test_cleanup_closed(loop, mocker) -> None:
     assert cleanup_closed_handle.cancel.called
 
 
-async def test_cleanup_closed_disabled(loop, mocker) -> None:
-    conn = aiohttp.BaseConnector(loop=loop, enable_cleanup_closed=False)
+async def test_cleanup_closed_is_noop_on_fixed_cpython() -> None:
+    """Ensure that enable_cleanup_closed is a noop on fixed Python versions."""
+    with mock.patch("aiohttp.connector.NEEDS_CLEANUP_CLOSED", False), pytest.warns(
+        DeprecationWarning, match="cleanup_closed ignored"
+    ):
+        conn = aiohttp.BaseConnector(enable_cleanup_closed=True)
+        assert conn._cleanup_closed_disabled is True
+
+
+async def test_cleanup_closed_disabled(
+    loop: asyncio.AbstractEventLoop, mocker: MockerFixture
+) -> None:
+    conn = aiohttp.BaseConnector(enable_cleanup_closed=False)
 
     tr = mock.Mock()
     conn._cleanup_closed_transports = [tr]
@@ -2293,8 +2320,11 @@ async def test_close_abort_closed_transports(loop: asyncio.AbstractEventLoop) ->
     assert conn.closed
 
 
-async def test_close_cancels_cleanup_closed_handle(loop) -> None:
-    conn = aiohttp.BaseConnector(loop=loop, enable_cleanup_closed=True)
+@pytest.mark.usefixtures("enable_cleanup_closed")
+async def test_close_cancels_cleanup_closed_handle(
+    loop: asyncio.AbstractEventLoop,
+) -> None:
+    conn = aiohttp.BaseConnector(enable_cleanup_closed=True)
     assert conn._cleanup_closed_handle is not None
     await conn.close()
     assert conn._cleanup_closed_handle is None

tests/test_proxy.py

@@ -384,7 +384,9 @@ async def make_conn():
         autospec=True,
         spec_set=True,
     )
-    def test_https_connect(self, start_connection: Any, ClientRequestMock: Any) -> None:
+    def test_https_connect(
+        self, start_connection: mock.Mock, ClientRequestMock: mock.Mock
+    ) -> None:
         proxy_req = ClientRequest(
             "GET", URL("http://proxy.example.com"), loop=self.loop
         )