diff --git a/CHANGES/10089.bugfix.rst b/CHANGES/10089.bugfix.rst new file mode 100644 index 00000000000..75f23db31b1 --- /dev/null +++ b/CHANGES/10089.bugfix.rst @@ -0,0 +1,2 @@ +Reduced redundant ``ClientResponse._release_connection()`` invocations so +successful requests now trigger a single release-path call per response. diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index 551b3374c6a..9e7d9ecba98 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -583,7 +583,8 @@ async def wait_for_close(self) -> None: and task.cancelling() ): raise - self.release() + if not self._released: + self.release() async def read(self) -> bytes: """Read response payload.""" diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index 971e7576d52..98f483c2680 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -370,6 +370,28 @@ async def handler(request: web.Request) -> web.Response: assert 1 == len(client._session.connector._conns) +async def test_release_connection_called_three_times_per_request( + aiohttp_client: AiohttpClient, mocker: MockerFixture +) -> None: + async def handler(request: web.Request) -> web.Response: + await request.read() + return web.Response(body=b"OK") + + app = web.Application() + app.router.add_route("GET", "/", handler) + client = await aiohttp_client(app) + + spy = mocker.spy(aiohttp.client_reqrep.ClientResponse, "_release_connection") + for _ in range(3): + async with client.get("/") as resp: + await resp.read() + await asyncio.sleep(0) + + # One request with response body read in a context manager currently hits: + # _response_eof(), _wait_released(), and release() from __aexit__(). + assert spy.call_count == 9 + + async def test_HTTP_304(aiohttp_client: AiohttpClient) -> None: async def handler(request: web.Request) -> web.Response: body = await request.read() diff --git a/tests/test_client_response.py b/tests/test_client_response.py index 9d7025efaca..9bfbef6188f 100644 --- a/tests/test_client_response.py +++ b/tests/test_client_response.py @@ -286,6 +286,78 @@ async def test_release(loop: asyncio.AbstractEventLoop, session: ClientSession) assert response._connection is None +async def test_wait_released_releases_connection( + loop: asyncio.AbstractEventLoop, session: ClientSession +) -> None: + url = URL("http://def-cl-resp.org") + response = ClientResponse( + "get", + url, + writer=None, + continue100=None, + timer=TimerNoop(), + traces=[], + loop=loop, + session=session, + request_headers=CIMultiDict[str](), + original_url=url, + ) + conn = response._connection = mock.Mock() + + await response._wait_released() + + conn.release.assert_called_once_with() + assert response._connection is None + + +async def test_wait_released_calls_release_without_connection( + loop: asyncio.AbstractEventLoop, session: ClientSession +) -> None: + url = URL("http://def-cl-resp.org") + response = ClientResponse( + "get", + url, + writer=None, + continue100=None, + timer=TimerNoop(), + traces=[], + loop=loop, + session=session, + request_headers=CIMultiDict[str](), + original_url=url, + ) + response._connection = None + + with mock.patch.object(response, "_release_connection") as release_connection: + await response._wait_released() + + release_connection.assert_called_once_with() + + +async def test_wait_for_close_skips_release_if_already_released( + loop: asyncio.AbstractEventLoop, session: ClientSession +) -> None: + url = URL("http://def-cl-resp.org") + response = ClientResponse( + "get", + url, + writer=None, + continue100=None, + timer=TimerNoop(), + traces=[], + loop=loop, + session=session, + request_headers=CIMultiDict[str](), + original_url=url, + ) + response._released = True + + with mock.patch.object(response, "release") as release: + await response.wait_for_close() + + release.assert_not_called() + + @pytest.mark.skipif( sys.implementation.name != "cpython", reason="Other implementations has different GC strategies",