From f5b653055f7ccc4a023fe08cbac502eb2c3c5569 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 15 Mar 2025 14:33:21 -1000 Subject: [PATCH] Re-raise OSError as ClientConnectionError when failing to explicitly close connector socket (#10551) ## What do these changes do? This is a followup to #10464 to handle the case where `socket.close()` can also raise. This matches the logic we have in aiohappyeyeballs: https://github.com/aio-libs/aiohappyeyeballs/blob/e3bd5bdf44f5d187802de6dcb08d27e1ca6da048/src/aiohappyeyeballs/impl.py#L227 We shouldn't raising `OSError` externally from this method as callers expect a `ClientError` ## Are there changes in behavior for the user? bugfix ## Is it a substantial burden for the maintainers to support this? no ## Related issue number fixes #10506 ## Checklist - [x] I think the code is well written - [x] Unit tests for the changes exist - [x] Documentation reflects the changes - [x] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` * The format is <Name> <Surname>. * Please keep alphabetical order, the file is sorted by names. - [x] Add a new news fragment into the `CHANGES/` folder * name it `..rst` (e.g. `588.bugfix.rst`) * if you don't have an issue number, change it to the pull request number after creating the PR * `.bugfix`: A bug fix for something the maintainers deemed an improper undesired behavior that got corrected to match pre-agreed expectations. * `.feature`: A new behavior, public APIs. That sort of stuff. * `.deprecation`: A declaration of future API removals and breaking changes in behavior. * `.breaking`: When something public is removed in a breaking way. Could be deprecated in an earlier release. * `.doc`: Notable updates to the documentation structure or build process. * `.packaging`: Notes for downstreams about unobvious side effects and tooling. Changes in the test invocation considerations and runtime assumptions. * `.contrib`: Stuff that affects the contributor experience. e.g. Running tests, building the docs, setting up the development environment. * `.misc`: Changes that are hard to assign to any of the above categories. * Make sure to use full sentences with correct case and punctuation, for example: ```rst Fixed issue with non-ascii contents in doctest text files -- by :user:`contributor-gh-handle`. ``` Use the past tense or the present tense a non-imperative mood, referring to what's changed compared to the last released version of this project. (cherry picked from commit d067260df75e4d04a23a0481cf9cf8f7194c80f1) --- CHANGES/10551.bugfix.rst | 1 + aiohttp/connector.py | 5 ++++- tests/test_connector.py | 27 +++++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 CHANGES/10551.bugfix.rst diff --git a/CHANGES/10551.bugfix.rst b/CHANGES/10551.bugfix.rst new file mode 100644 index 00000000000..8f3eb24d6ae --- /dev/null +++ b/CHANGES/10551.bugfix.rst @@ -0,0 +1 @@ +The connector now raises :exc:`aiohttp.ClientConnectionError` instead of :exc:`OSError` when failing to explicitly close the socket after :py:meth:`asyncio.loop.create_connection` fails -- by :user:`bdraco`. diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 081ff330d38..de9062e8ae3 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -1150,7 +1150,10 @@ async def _wrap_create_connection( # 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() + 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 b199d9b5703..2aaa50985a1 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -640,6 +640,33 @@ async def test_tcp_connector_closes_socket_on_error( 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: