From c3a9c3e67b6ad39e28095fd478e86053b34a7e45 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 9 Nov 2024 13:34:27 +0000 Subject: [PATCH] Make ``enable_cleanup_closed`` a NOOP for Python 3.12.7+ and 3.13.1+ (#9726) --- CHANGES/9726.misc.rst | 1 + aiohttp/connector.py | 19 +++++++++++++++++++ docs/client_reference.rst | 6 +++++- tests/conftest.py | 13 ++++++++++++- tests/test_connector.py | 15 +++++++++++++++ tests/test_proxy.py | 2 ++ 6 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 CHANGES/9726.misc.rst diff --git a/CHANGES/9726.misc.rst b/CHANGES/9726.misc.rst new file mode 100644 index 00000000000..460c48b7995 --- /dev/null +++ b/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`. diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 8464b3a9d46..f746b633d80 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -77,6 +77,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") @@ -270,6 +278,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]] = [] diff --git a/docs/client_reference.rst b/docs/client_reference.rst index dfe93bda0d9..937e5e8afbb 100644 --- a/docs/client_reference.rst +++ b/docs/client_reference.rst @@ -963,10 +963,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` used for handling connections. diff --git a/tests/conftest.py b/tests/conftest.py index 5168ba8e51e..a953bbe4aad 100644 --- a/tests/conftest.py +++ b/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, Callable, Iterator +from typing import Any, Callable, Generator, Iterator from unittest import mock from uuid import uuid4 @@ -239,3 +239,14 @@ def key(key_data: bytes) -> bytes: @pytest.fixture def ws_key(key: bytes) -> str: 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 diff --git a/tests/test_connector.py b/tests/test_connector.py index 7abe5131e91..5eb508fad11 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -377,6 +377,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) @@ -440,6 +441,7 @@ async def test_release(loop: asyncio.AbstractEventLoop, key: ConnectionKey) -> N await conn.close() +@pytest.mark.usefixtures("enable_cleanup_closed") async def test_release_ssl_transport( loop: asyncio.AbstractEventLoop, ssl_key: ConnectionKey ) -> None: @@ -1797,6 +1799,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 @@ -1828,6 +1831,7 @@ async def test_cleanup(key: ConnectionKey) -> None: assert conn._cleanup_handle is None +@pytest.mark.usefixtures("enable_cleanup_closed") async def test_cleanup_close_ssl_transport( loop: asyncio.AbstractEventLoop, ssl_key: ConnectionKey ) -> None: @@ -1897,6 +1901,7 @@ async def test_cleanup3(loop: asyncio.AbstractEventLoop, key: ConnectionKey) -> await conn.close() +@pytest.mark.usefixtures("enable_cleanup_closed") async def test_cleanup_closed( loop: asyncio.AbstractEventLoop, mocker: MockerFixture ) -> None: @@ -1916,6 +1921,15 @@ async def test_cleanup_closed( assert cleanup_closed_handle.cancel.called +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: @@ -2440,6 +2454,7 @@ async def test_close_abort_closed_transports(loop: asyncio.AbstractEventLoop) -> assert conn.closed +@pytest.mark.usefixtures("enable_cleanup_closed") async def test_close_cancels_cleanup_closed_handle( loop: asyncio.AbstractEventLoop, ) -> None: diff --git a/tests/test_proxy.py b/tests/test_proxy.py index 47b33e805f3..ec990792369 100644 --- a/tests/test_proxy.py +++ b/tests/test_proxy.py @@ -5,6 +5,7 @@ import unittest from unittest import mock +import pytest from yarl import URL import aiohttp @@ -421,6 +422,7 @@ async def make_conn() -> aiohttp.TCPConnector: autospec=True, spec_set=True, ) + @pytest.mark.usefixtures("enable_cleanup_closed") def test_https_connect_fingerprint_mismatch( self, start_connection: mock.Mock, ClientRequestMock: mock.Mock ) -> None: