From 1928761ed5385ee543b112b857ca2f05f6a0082c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 31 Mar 2025 15:43:51 -1000 Subject: [PATCH 1/3] Revert: Close the socket if there's a failure in start_connection() #10464 fixes #10617 alternative fix is https://github.com/MagicStack/uvloop/pull/646 --- aiohttp/connector.py | 16 +------------ tests/test_connector.py | 50 ----------------------------------------- 2 files changed, 1 insertion(+), 65 deletions(-) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index b61a4da33a3..08e6ae275ed 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -1119,7 +1119,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 @@ -1132,11 +1131,7 @@ async def _wrap_create_connection( loop=self._loop, socket_factory=self._socket_factory, ) - 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: @@ -1145,15 +1140,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 def _warn_about_tls_in_tls( self, diff --git a/tests/test_connector.py b/tests/test_connector.py index aac122c7119..c4019df3cdf 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -646,56 +646,6 @@ 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, - ), - 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: asyncio.AbstractEventLoop, start_connection: mock.AsyncMock ) -> None: From c491c212fb5a1eeaec46c72bc9ad2d5b07377f4d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 31 Mar 2025 15:47:14 -1000 Subject: [PATCH 2/3] changelog --- CHANGES/10464.bugfix.rst | 1 + CHANGES/10617.bugfix.rst | 1 + CHANGES/10656.bugfix.rst | 3 +++ 3 files changed, 5 insertions(+) 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..2807c94b019 --- /dev/null +++ b/CHANGES/10656.bugfix.rst @@ -0,0 +1,3 @@ +Reverted explicitly close sockets if an exception is raised -- by :user:`bdraco`. + +This change originally appeared in aiohttp 3.11.13 From bb6390966bec4a6c97d5f1e407ee50d491c40ae5 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 31 Mar 2025 15:48:11 -1000 Subject: [PATCH 3/3] tweak --- CHANGES/10656.bugfix.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES/10656.bugfix.rst b/CHANGES/10656.bugfix.rst index 2807c94b019..ec3853107ad 100644 --- a/CHANGES/10656.bugfix.rst +++ b/CHANGES/10656.bugfix.rst @@ -1,3 +1,3 @@ -Reverted explicitly close sockets if an exception is raised -- by :user:`bdraco`. +Reverted explicitly closing sockets if an exception is raised during ``create_connection`` -- by :user:`bdraco`. This change originally appeared in aiohttp 3.11.13