From 623ca6f20b8b0b0a53e5ca691143526ee7c5fa02 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 23 Nov 2023 17:24:53 +0000 Subject: [PATCH 01/17] Fix regression with wait_until_eof=False --- aiohttp/client_reqrep.py | 3 ++- tests/test_client_functional.py | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index 21858fc345a..818136314a4 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -1048,7 +1048,8 @@ async def read(self) -> bytes: elif self._released: # Response explicitly released raise ClientConnectionError("Connection closed") - await self._wait_released() # Underlying connection released + if self._protocol._parser.read_until_eof: + await self._wait_released() # Underlying connection released return self._body def get_encoding(self) -> str: diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index 4f0d594cb1f..1bbed08ccad 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -174,6 +174,25 @@ async def handler(request): assert 1 == len(client._session.connector._conns) +async def test_no_eof_response_not_released(aiohttp_client: Any) -> None: + async def handler(request): + body = await request.read() + assert b"" == body + return web.Response(body=b"OK" * 2000) + + app = web.Application() + app.router.add_route("GET", "/", handler) + + client = await aiohttp_client(app) + + resp = await client.get("/") + await resp.read() + assert resp.connection is None + resp = await client.get("/", read_until_eof=False) + await resp.read() + assert resp.connection is not None + + async def test_keepalive_server_force_close_connection(aiohttp_client: Any) -> None: async def handler(request): body = await request.read() From 99f21666420d7deae825be379ee5d05885f2d122 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 23 Nov 2023 17:27:11 +0000 Subject: [PATCH 02/17] Create 7879.bugfix --- CHANGES/7879.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 CHANGES/7879.bugfix diff --git a/CHANGES/7879.bugfix b/CHANGES/7879.bugfix new file mode 100644 index 00000000000..57156f265d0 --- /dev/null +++ b/CHANGES/7879.bugfix @@ -0,0 +1 @@ +Fixed a regression when using `wait_until_eof=True` in a request method. -- by :user:`Dreamsorcerer` From 23c1c915cb7d9cdd5a635e807ae4f95bb8a3cfcd Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 25 Nov 2023 17:24:05 +0000 Subject: [PATCH 03/17] Fix properly --- aiohttp/client_reqrep.py | 21 ++++++++------------- tests/test_client_functional.py | 7 ++----- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index 818136314a4..f5ea60ea3de 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -940,19 +940,9 @@ def _response_eof(self) -> None: if self._closed: return - if self._connection is not None: - # websocket, protocol could be None because - # connection could be detached - if ( - self._connection.protocol is not None - and self._connection.protocol.upgraded - ): - return - - self._release_connection() - self._closed = True self._cleanup_writer() + self._release_connection() @property def closed(self) -> bool: @@ -978,7 +968,7 @@ def release(self) -> Any: self._closed = True self._cleanup_writer() - self._release_connection() + self._release_connection(skip_upgraded=False) return noop() @property @@ -1003,8 +993,13 @@ def raise_for_status(self) -> None: headers=self.headers, ) - def _release_connection(self) -> None: + def _release_connection(self, skip_upgraded: bool = True) -> None: if self._connection is not None: + # protocol could be None because connection could be detached + protocol = self._connection.protocol + if skip_upgraded and protocol is not None and protocol.upgraded: + return + if self._writer is None: self._connection.release() self._connection = None diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index 1bbed08ccad..a3bb1650792 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -174,11 +174,11 @@ async def handler(request): assert 1 == len(client._session.connector._conns) -async def test_no_eof_response_not_released(aiohttp_client: Any) -> None: +async def test_upgrade_connection_not_released_after_read(aiohttp_client) -> None: async def handler(request): body = await request.read() assert b"" == body - return web.Response(body=b"OK" * 2000) + return web.Response(status=101, headers={"Connection": "Upgrade"}) app = web.Application() app.router.add_route("GET", "/", handler) @@ -187,9 +187,6 @@ async def handler(request): resp = await client.get("/") await resp.read() - assert resp.connection is None - resp = await client.get("/", read_until_eof=False) - await resp.read() assert resp.connection is not None From 3c12c248f862405648548a6cb52ca33ae1f14db7 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 25 Nov 2023 17:25:55 +0000 Subject: [PATCH 04/17] Update 7879.bugfix --- CHANGES/7879.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES/7879.bugfix b/CHANGES/7879.bugfix index 57156f265d0..08baf85be42 100644 --- a/CHANGES/7879.bugfix +++ b/CHANGES/7879.bugfix @@ -1 +1 @@ -Fixed a regression when using `wait_until_eof=True` in a request method. -- by :user:`Dreamsorcerer` +Fixed a regression where connection may get closed during upgrade. -- by :user:`Dreamsorcerer` From 3abf48083e7052684b499351389894b44dca707f Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 25 Nov 2023 17:33:43 +0000 Subject: [PATCH 05/17] Revert previous fix --- aiohttp/client_reqrep.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index f5ea60ea3de..d4e6a696148 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -1043,8 +1043,7 @@ async def read(self) -> bytes: elif self._released: # Response explicitly released raise ClientConnectionError("Connection closed") - if self._protocol._parser.read_until_eof: - await self._wait_released() # Underlying connection released + await self._wait_released() # Underlying connection released return self._body def get_encoding(self) -> str: From d64b88425308acc7b6640d307db9a1b5006ae869 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 25 Nov 2023 17:35:18 +0000 Subject: [PATCH 06/17] Update tests/test_client_functional.py --- tests/test_client_functional.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index a3bb1650792..17d6d6a05a2 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -174,7 +174,7 @@ async def handler(request): assert 1 == len(client._session.connector._conns) -async def test_upgrade_connection_not_released_after_read(aiohttp_client) -> None: +async def test_upgrade_connection_not_released_after_read(aiohttp_client: Any) -> None: async def handler(request): body = await request.read() assert b"" == body From af52f618f8efc5c5167a305fe3d28c7926aef6a6 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 25 Nov 2023 17:35:49 +0000 Subject: [PATCH 07/17] Update tests/test_client_functional.py --- tests/test_client_functional.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index 17d6d6a05a2..c28ce8a78a3 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -175,7 +175,7 @@ async def handler(request): async def test_upgrade_connection_not_released_after_read(aiohttp_client: Any) -> None: - async def handler(request): + async def handler(request: web.Request) -> web.Response: body = await request.read() assert b"" == body return web.Response(status=101, headers={"Connection": "Upgrade"}) From 3e3a910c10604ac0d4f9cfa4a1acf54dd45fd0a5 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 25 Nov 2023 18:48:38 +0000 Subject: [PATCH 08/17] Update tests/test_client_functional.py --- tests/test_client_functional.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index c28ce8a78a3..a06f7f4ce48 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -178,7 +178,7 @@ async def test_upgrade_connection_not_released_after_read(aiohttp_client: Any) - async def handler(request: web.Request) -> web.Response: body = await request.read() assert b"" == body - return web.Response(status=101, headers={"Connection": "Upgrade"}) + return web.Response(status=101, headers={"Connection": "Upgrade", "Upgrade": "tcp"}) app = web.Application() app.router.add_route("GET", "/", handler) From cef6586445aab5ee61916e622684b2de6416922c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 25 Nov 2023 18:49:08 +0000 Subject: [PATCH 09/17] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_client_functional.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index a06f7f4ce48..42d217fd101 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -178,7 +178,9 @@ async def test_upgrade_connection_not_released_after_read(aiohttp_client: Any) - async def handler(request: web.Request) -> web.Response: body = await request.read() assert b"" == body - return web.Response(status=101, headers={"Connection": "Upgrade", "Upgrade": "tcp"}) + return web.Response( + status=101, headers={"Connection": "Upgrade", "Upgrade": "tcp"} + ) app = web.Application() app.router.add_route("GET", "/", handler) From 34e07f8255944bbb21a45a3d192da58b49236da4 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 25 Nov 2023 22:40:51 +0000 Subject: [PATCH 10/17] Ensure not closed --- aiohttp/client_reqrep.py | 19 +++++++++++-------- tests/test_client_functional.py | 1 + 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index d4e6a696148..bfedf978e12 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -940,6 +940,12 @@ def _response_eof(self) -> None: if self._closed: return + if self._connection is not None: + # protocol could be None because connection could be detached + protocol = self._connection.protocol + if protocol is not None and protocol.upgraded: + return + self._closed = True self._cleanup_writer() self._release_connection() @@ -968,7 +974,7 @@ def release(self) -> Any: self._closed = True self._cleanup_writer() - self._release_connection(skip_upgraded=False) + self._release_connection() return noop() @property @@ -993,13 +999,8 @@ def raise_for_status(self) -> None: headers=self.headers, ) - def _release_connection(self, skip_upgraded: bool = True) -> None: + def _release_connection(self) -> None: if self._connection is not None: - # protocol could be None because connection could be detached - protocol = self._connection.protocol - if skip_upgraded and protocol is not None and protocol.upgraded: - return - if self._writer is None: self._connection.release() self._connection = None @@ -1043,7 +1044,9 @@ async def read(self) -> bytes: elif self._released: # Response explicitly released raise ClientConnectionError("Connection closed") - await self._wait_released() # Underlying connection released + protocol = self._connection.protocol + if protocol is None or not protocol.upgraded: + await self._wait_released() # Underlying connection released return self._body def get_encoding(self) -> str: diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index 42d217fd101..66af22b4494 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -190,6 +190,7 @@ async def handler(request: web.Request) -> web.Response: resp = await client.get("/") await resp.read() assert resp.connection is not None + assert not resp.closed async def test_keepalive_server_force_close_connection(aiohttp_client: Any) -> None: From a3ff38e54e53597e0b7f9371453fbecf652bcefb Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 25 Nov 2023 22:43:10 +0000 Subject: [PATCH 11/17] Tweak --- aiohttp/client_reqrep.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index bfedf978e12..c0319074b65 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -940,11 +940,10 @@ def _response_eof(self) -> None: if self._closed: return - if self._connection is not None: - # protocol could be None because connection could be detached - protocol = self._connection.protocol - if protocol is not None and protocol.upgraded: - return + # protocol could be None because connection could be detached + protocol = getattr(self._connection, "protocol", None) + if protocol is not None and protocol.upgraded: + return self._closed = True self._cleanup_writer() @@ -1044,7 +1043,7 @@ async def read(self) -> bytes: elif self._released: # Response explicitly released raise ClientConnectionError("Connection closed") - protocol = self._connection.protocol + protocol = getattr(self._connection, "protocol", None) if protocol is None or not protocol.upgraded: await self._wait_released() # Underlying connection released return self._body From ae2bae0ed792ae8f82e4e448ff4e30eea9ae4433 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 25 Nov 2023 22:53:05 +0000 Subject: [PATCH 12/17] Tweak --- aiohttp/client_reqrep.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index c0319074b65..2185aa85738 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -941,7 +941,7 @@ def _response_eof(self) -> None: return # protocol could be None because connection could be detached - protocol = getattr(self._connection, "protocol", None) + protocol = self._connection and self._connection.protocol if protocol is not None and protocol.upgraded: return @@ -1043,7 +1043,7 @@ async def read(self) -> bytes: elif self._released: # Response explicitly released raise ClientConnectionError("Connection closed") - protocol = getattr(self._connection, "protocol", None) + protocol = self._connection and self._connection.protocol if protocol is None or not protocol.upgraded: await self._wait_released() # Underlying connection released return self._body From 028b45a3bafcacf01305347be7f6891d0dfaa18e Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 25 Nov 2023 23:12:05 +0000 Subject: [PATCH 13/17] Ignore --- aiohttp/client_reqrep.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index 2185aa85738..623c99d8bc4 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -941,7 +941,8 @@ def _response_eof(self) -> None: return # protocol could be None because connection could be detached - protocol = self._connection and self._connection.protocol + # Mypy bug: https://github.com/python/mypy/issues/16565 + protocol = self._connection and self._connection.protocol # type: ignore[union-attr] if protocol is not None and protocol.upgraded: return @@ -1043,7 +1044,8 @@ async def read(self) -> bytes: elif self._released: # Response explicitly released raise ClientConnectionError("Connection closed") - protocol = self._connection and self._connection.protocol + # Mypy bug: https://github.com/python/mypy/issues/16565 + protocol = self._connection and self._connection.protocol # type: ignore[union-attr] if protocol is None or not protocol.upgraded: await self._wait_released() # Underlying connection released return self._body From b18f4a206bd6fbe4cab0bff21399620a93d10ea9 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 25 Nov 2023 23:14:33 +0000 Subject: [PATCH 14/17] Wrong line --- aiohttp/client_reqrep.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index 623c99d8bc4..ea3d2eaef45 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -941,9 +941,9 @@ def _response_eof(self) -> None: return # protocol could be None because connection could be detached + protocol = self._connection and self._connection.protocol # Mypy bug: https://github.com/python/mypy/issues/16565 - protocol = self._connection and self._connection.protocol # type: ignore[union-attr] - if protocol is not None and protocol.upgraded: + if protocol is not None and protocol.upgraded: # type: ignore[union-attr] return self._closed = True @@ -1044,9 +1044,9 @@ async def read(self) -> bytes: elif self._released: # Response explicitly released raise ClientConnectionError("Connection closed") + protocol = self._connection and self._connection.protocol # Mypy bug: https://github.com/python/mypy/issues/16565 - protocol = self._connection and self._connection.protocol # type: ignore[union-attr] - if protocol is None or not protocol.upgraded: + if protocol is None or not protocol.upgraded: # type: ignore[union-attr] await self._wait_released() # Underlying connection released return self._body From 91beedf6537c51c0df7e44257173b4aae0e8a3d2 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sun, 26 Nov 2023 13:34:27 +0000 Subject: [PATCH 15/17] Fix type error --- aiohttp/client_reqrep.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index ea3d2eaef45..76b68993431 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -941,9 +941,8 @@ def _response_eof(self) -> None: return # protocol could be None because connection could be detached - protocol = self._connection and self._connection.protocol - # Mypy bug: https://github.com/python/mypy/issues/16565 - if protocol is not None and protocol.upgraded: # type: ignore[union-attr] + protocol = (self._connection or None) and self._connection.protocol + if protocol is not None and protocol.upgraded: return self._closed = True @@ -1044,9 +1043,8 @@ async def read(self) -> bytes: elif self._released: # Response explicitly released raise ClientConnectionError("Connection closed") - protocol = self._connection and self._connection.protocol - # Mypy bug: https://github.com/python/mypy/issues/16565 - if protocol is None or not protocol.upgraded: # type: ignore[union-attr] + protocol = (self._connection or None) and self._connection.protocol + if protocol is None or not protocol.upgraded: await self._wait_released() # Underlying connection released return self._body From 828af696266cb67a2bad8536267c644c1fcd3e68 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sun, 26 Nov 2023 14:02:45 +0000 Subject: [PATCH 16/17] Update connector.py --- aiohttp/connector.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 2460ca46705..295882f65ca 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -107,6 +107,10 @@ def __del__(self, _warnings: Any = warnings) -> None: context["source_traceback"] = self._source_traceback self._loop.call_exception_handler(context) + def __bool__(self) -> Literal[True]: + """Force subclasses to not be falsy, to make checks simpler.""" + return True + @property def transport(self) -> Optional[asyncio.Transport]: if self._protocol is None: From 2ddee9f04f1a5de4d10b427549930d268abbb7e8 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sun, 26 Nov 2023 14:03:29 +0000 Subject: [PATCH 17/17] Update client_reqrep.py --- aiohttp/client_reqrep.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index 76b68993431..2185aa85738 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -941,7 +941,7 @@ def _response_eof(self) -> None: return # protocol could be None because connection could be detached - protocol = (self._connection or None) and self._connection.protocol + protocol = self._connection and self._connection.protocol if protocol is not None and protocol.upgraded: return @@ -1043,7 +1043,7 @@ async def read(self) -> bytes: elif self._released: # Response explicitly released raise ClientConnectionError("Connection closed") - protocol = (self._connection or None) and self._connection.protocol + protocol = self._connection and self._connection.protocol if protocol is None or not protocol.upgraded: await self._wait_released() # Underlying connection released return self._body