From 8221b0911ded789c3e8261f2d0916f0de686667e Mon Sep 17 00:00:00 2001 From: Andrew Top Date: Fri, 14 Feb 2025 05:05:32 +0000 Subject: [PATCH 1/9] Close the socket if there's a failure in start_connection() --- aiohttp/connector.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 263a4823334..7fd6aaf0709 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -1101,6 +1101,7 @@ async def _wrap_create_connection( client_error: Type[Exception] = ClientConnectorError, **kwargs: Any, ) -> Tuple[asyncio.Transport, ResponseHandler]: + sock = None try: async with ceil_timeout( timeout.sock_connect, ceil_threshold=timeout.ceil_threshold @@ -1112,7 +1113,11 @@ async def _wrap_create_connection( interleave=self._interleave, loop=self._loop, ) - return await self._loop.create_connection(*args, **kwargs, sock=sock) + connection = await self._loop.create_connection( + *args, **kwargs, sock=sock + ) + sock = None + return connection except cert_errors as exc: raise ClientConnectorCertificateError(req.connection_key, exc) from exc except ssl_errors as exc: @@ -1121,6 +1126,9 @@ async def _wrap_create_connection( if exc.errno is None and isinstance(exc, asyncio.TimeoutError): raise raise client_error(req.connection_key, exc) from exc + finally: + if sock is not None: + sock.close() def _warn_about_tls_in_tls( self, From 2d1989d04cdcc900bf2028f20c4761fb99653256 Mon Sep 17 00:00:00 2001 From: Andrew Top Date: Sun, 16 Feb 2025 00:52:19 +0000 Subject: [PATCH 2/9] Update start_connection mock to return a mocked socket --- tests/conftest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/conftest.py b/tests/conftest.py index e24d05b31ff..6e4a93d2937 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -222,6 +222,7 @@ def start_connection() -> Iterator[mock.Mock]: "aiohttp.connector.aiohappyeyeballs.start_connection", autospec=True, spec_set=True, + return_value=mock.create_autospec(socket.socket, spec_set=True, instance=True), ) as start_connection_mock: yield start_connection_mock From e3857e47eeef757234a96447409bc22cb3703be7 Mon Sep 17 00:00:00 2001 From: Andrew Top Date: Sun, 16 Feb 2025 01:04:40 +0000 Subject: [PATCH 3/9] Fix proxy test. --- tests/test_proxy.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_proxy.py b/tests/test_proxy.py index 2aaafedeb5c..2359f5bf17b 100644 --- a/tests/test_proxy.py +++ b/tests/test_proxy.py @@ -216,6 +216,7 @@ async def make_conn() -> aiohttp.TCPConnector: "aiohttp.connector.aiohappyeyeballs.start_connection", autospec=True, spec_set=True, + return_value=mock.create_autospec(socket.socket, spec_set=True, instance=True), ) def test_proxy_connection_error(self, start_connection: mock.Mock) -> None: async def make_conn() -> aiohttp.TCPConnector: From 8af0f246e2ef395d28172195b4a59d12dc9b56cc Mon Sep 17 00:00:00 2001 From: Andrew Top Date: Sun, 16 Feb 2025 01:04:57 +0000 Subject: [PATCH 4/9] Add sock.close() test. --- tests/test_connector.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/test_connector.py b/tests/test_connector.py index 61b452eee81..e0a27f49800 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -646,6 +646,27 @@ async def test_tcp_connector_certificate_error( await conn.close() +async def test_tcp_connector_closes_socket_on_error( + loop: asyncio.AbstractEventLoop, start_connection: mock.AsyncMock +) -> None: + req = ClientRequest("GET", URL("https://127.0.0.1:443"), loop=loop) + + conn = aiohttp.TCPConnector() + with mock.patch.object( + conn._loop, + "create_connection", + autospec=True, + spec_set=True, + side_effect=ValueError, + ): + with pytest.raises(ValueError): + await conn.connect(req, [], ClientTimeout()) + + assert start_connection.return_value.close.called + + await conn.close() + + async def test_tcp_connector_server_hostname_default( loop: asyncio.AbstractEventLoop, start_connection: mock.AsyncMock ) -> None: From d9ae936f6543077cdb4d6a9ab86dbb7230bab925 Mon Sep 17 00:00:00 2001 From: Andrew Top Date: Sun, 16 Feb 2025 01:11:15 +0000 Subject: [PATCH 5/9] Add a comment. --- aiohttp/connector.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 7fd6aaf0709..21aedb46535 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -1128,6 +1128,9 @@ async def _wrap_create_connection( raise client_error(req.connection_key, exc) from exc finally: if sock is not None: + # Will be hit if an exception is thrown before the event loop takes the socket. + # In that case, proactively close the socket to guard against event loop leaks. + # For example, see https://github.com/MagicStack/uvloop/issues/653. sock.close() def _warn_about_tls_in_tls( From 5a8afa5523466ed01964484bb5ab32a0bf8687ad Mon Sep 17 00:00:00 2001 From: Andrew Top Date: Sun, 16 Feb 2025 01:27:14 +0000 Subject: [PATCH 6/9] Doc changes --- CHANGES/10464.bugfix.rst | 1 + CONTRIBUTORS.txt | 1 + 2 files changed, 2 insertions(+) create mode 100644 CHANGES/10464.bugfix.rst diff --git a/CHANGES/10464.bugfix.rst b/CHANGES/10464.bugfix.rst new file mode 100644 index 00000000000..ddd3d4878b5 --- /dev/null +++ b/CHANGES/10464.bugfix.rst @@ -0,0 +1 @@ +Changed connection creation to explicitly close sockets if an exception is raised in the event loop's ``create_connection`` method. diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 03e35ce789f..19cca5b8c99 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -43,6 +43,7 @@ Andrej Antonov Andrew Leech Andrew Lytvyn Andrew Svetlov +Andrew Top Andrew Zhou Andrii Soldatenko Anes Abismail From 6abaae78e662c9fe6e2ff084c58b7bb21336d34e Mon Sep 17 00:00:00 2001 From: Andrew Top Date: Tue, 18 Feb 2025 06:30:31 +0000 Subject: [PATCH 7/9] Review comments -- add author to changes --- CHANGES/10464.bugfix.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES/10464.bugfix.rst b/CHANGES/10464.bugfix.rst index ddd3d4878b5..4e21000a317 100644 --- a/CHANGES/10464.bugfix.rst +++ b/CHANGES/10464.bugfix.rst @@ -1 +1 @@ -Changed connection creation to explicitly close sockets if an exception is raised in the event loop's ``create_connection`` method. +Changed connection creation to explicitly close sockets if an exception is raised in the event loop's ``create_connection`` method -- by :user:`top-oai`. From 2d0d4abc40dd9c1a86470f850d2e1a9037564671 Mon Sep 17 00:00:00 2001 From: Andrew Top Date: Tue, 18 Feb 2025 06:32:51 +0000 Subject: [PATCH 8/9] review feedback -- combine with clauses --- tests/test_connector.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tests/test_connector.py b/tests/test_connector.py index e0a27f49800..ebdb1a6b513 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -652,17 +652,19 @@ async def test_tcp_connector_closes_socket_on_error( req = ClientRequest("GET", URL("https://127.0.0.1:443"), loop=loop) conn = aiohttp.TCPConnector() - with mock.patch.object( - conn._loop, - "create_connection", - autospec=True, - spec_set=True, - side_effect=ValueError, + with ( + mock.patch.object( + conn._loop, + "create_connection", + autospec=True, + spec_set=True, + side_effect=ValueError, + ), + pytest.raises(ValueError), ): - with pytest.raises(ValueError): - await conn.connect(req, [], ClientTimeout()) + await conn.connect(req, [], ClientTimeout()) - assert start_connection.return_value.close.called + assert start_connection.return_value.close.called await conn.close() From b3fcd38747bcb291bc2cde5b69848575fa4d8520 Mon Sep 17 00:00:00 2001 From: Andrew Top Date: Tue, 18 Feb 2025 06:39:50 +0000 Subject: [PATCH 9/9] review feedback -- add explicit typing --- aiohttp/connector.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 21aedb46535..99b281b24a5 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -1101,7 +1101,7 @@ async def _wrap_create_connection( client_error: Type[Exception] = ClientConnectorError, **kwargs: Any, ) -> Tuple[asyncio.Transport, ResponseHandler]: - sock = None + sock: Union[socket.socket, None] = None try: async with ceil_timeout( timeout.sock_connect, ceil_threshold=timeout.ceil_threshold