From d1fa9a2054877e08299cea8855582000ddae4d01 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 31 Mar 2025 16:51:10 -1000 Subject: [PATCH] Revert: Close the socket if there's a failure in start_connection() #10464 (#10656) Reverts #10464 While this change improved the situation for uvloop users, it caused a regression with `SelectorEventLoop` (issue #10617) The alternative fix is https://github.com/MagicStack/uvloop/pull/646 (not merged at the time of this PR) issue #10617 appears to be very similar to https://github.com/python/cpython/commit/d5aeccf9767c1619faa29e8ed61c93bde7bc5e3f If someone can come up with a working reproducer for #10617 we can revisit this. cc @top-oai Minimal implementation that shows on cancellation the socket is cleaned up without the explicit `close` https://github.com/aio-libs/aiohttp/issues/10617#issuecomment-2767890703 so this should be unneeded unless I've missed something (very possible with all the moving parts here) ## Related issue number fixes #10617 (cherry picked from commit 06db052eae399de1c7c34c0122d736e06c045ec7) --- CHANGES/10464.bugfix.rst | 1 + CHANGES/10617.bugfix.rst | 1 + CHANGES/10656.bugfix.rst | 3 +++ aiohttp/connector.py | 16 +------------ tests/test_connector.py | 50 ---------------------------------------- 5 files changed, 6 insertions(+), 65 deletions(-) create mode 120000 CHANGES/10464.bugfix.rst create mode 120000 CHANGES/10617.bugfix.rst create mode 100644 CHANGES/10656.bugfix.rst diff --git a/CHANGES/10464.bugfix.rst b/CHANGES/10464.bugfix.rst new file mode 120000 index 00000000000..18996eb3cac --- /dev/null +++ b/CHANGES/10464.bugfix.rst @@ -0,0 +1 @@ +10656.bugfix.rst \ No newline at end of file diff --git a/CHANGES/10617.bugfix.rst b/CHANGES/10617.bugfix.rst new file mode 120000 index 00000000000..18996eb3cac --- /dev/null +++ b/CHANGES/10617.bugfix.rst @@ -0,0 +1 @@ +10656.bugfix.rst \ No newline at end of file diff --git a/CHANGES/10656.bugfix.rst b/CHANGES/10656.bugfix.rst new file mode 100644 index 00000000000..ec3853107ad --- /dev/null +++ b/CHANGES/10656.bugfix.rst @@ -0,0 +1,3 @@ +Reverted explicitly closing sockets if an exception is raised during ``create_connection`` -- by :user:`bdraco`. + +This change originally appeared in aiohttp 3.11.13 diff --git a/aiohttp/connector.py b/aiohttp/connector.py index e5cf3674cba..7420bd6070a 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -1108,7 +1108,6 @@ async def _wrap_create_connection( client_error: Type[Exception] = ClientConnectorError, **kwargs: Any, ) -> Tuple[asyncio.Transport, ResponseHandler]: - sock: Union[socket.socket, None] = None try: async with ceil_timeout( timeout.sock_connect, ceil_threshold=timeout.ceil_threshold @@ -1120,11 +1119,7 @@ async def _wrap_create_connection( interleave=self._interleave, loop=self._loop, ) - connection = await self._loop.create_connection( - *args, **kwargs, sock=sock - ) - sock = None - return connection + return await self._loop.create_connection(*args, **kwargs, sock=sock) except cert_errors as exc: raise ClientConnectorCertificateError(req.connection_key, exc) from exc except ssl_errors as exc: @@ -1133,15 +1128,6 @@ 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: - # 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. - try: - sock.close() - except OSError as exc: - raise client_error(req.connection_key, exc) from exc async def _wrap_existing_connection( self, diff --git a/tests/test_connector.py b/tests/test_connector.py index a86a2417423..a3fffc447ae 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -617,56 +617,6 @@ async def certificate_error(*args, **kwargs): 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, - ), - pytest.raises(ValueError), - ): - await conn.connect(req, [], ClientTimeout()) - - assert start_connection.return_value.close.called - - await conn.close() - - -async def test_tcp_connector_closes_socket_on_error_results_in_another_error( - loop: asyncio.AbstractEventLoop, start_connection: mock.AsyncMock -) -> None: - """Test that when error occurs while closing the socket.""" - req = ClientRequest("GET", URL("https://127.0.0.1:443"), loop=loop) - start_connection.return_value.close.side_effect = OSError( - 1, "error from closing socket" - ) - - conn = aiohttp.TCPConnector() - with ( - mock.patch.object( - conn._loop, - "create_connection", - autospec=True, - spec_set=True, - side_effect=ValueError, - ), - pytest.raises(aiohttp.ClientConnectionError, match="error from closing socket"), - ): - await conn.connect(req, [], ClientTimeout()) - - assert start_connection.return_value.close.called - - await conn.close() - - async def test_tcp_connector_server_hostname_default( loop: Any, start_connection: mock.AsyncMock ) -> None: