From 33fb186c24ec09b1cfdd7c4b47da6d000e18c30f Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 5 Sep 2024 17:59:14 +0100 Subject: [PATCH 01/15] Fix cancellations being swallowed --- aiohttp/client_reqrep.py | 17 +++++++++++------ aiohttp/web_protocol.py | 35 ++++++++++++++++++++++++---------- tests/test_client_request.py | 18 ++++++++++++++++++ tests/test_web_functional.py | 37 ++++++++++++++++++++++++++++++++++++ 4 files changed, 91 insertions(+), 16 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index c7512758f49..817e596edea 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -561,11 +561,8 @@ async def write_bytes( """Support coroutines that yields bytes objects.""" # 100 response if self._continue is not None: - try: - await writer.drain() - await self._continue - except asyncio.CancelledError: - return + await writer.drain() + await self._continue protocol = conn.protocol assert protocol is not None @@ -594,6 +591,7 @@ async def write_bytes( except asyncio.CancelledError: # Body hasn't been fully sent, so connection can't be reused. conn.close() + raise except Exception as underlying_exc: set_exception( protocol, @@ -696,8 +694,15 @@ async def send(self, conn: "Connection") -> "ClientResponse": async def close(self) -> None: if self._writer is not None: - with contextlib.suppress(asyncio.CancelledError): + try: await self._writer + except asyncio.CancelledError: + if ( + sys.version_info >= (3, 11) + and (task := asyncio.current_task()) + and task.cancelling() + ): + raise def terminate(self) -> None: if self._writer is not None: diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index 4ea0706e3ac..759b0447e75 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -285,17 +285,31 @@ async def shutdown(self, timeout: Optional[float] = 15.0) -> None: # Wait for graceful handler completion if self._handler_waiter is not None: - with suppress(asyncio.CancelledError, asyncio.TimeoutError): + try: async with ceil_timeout(timeout): await self._handler_waiter + except (asyncio.CancelledError, asyncio.TimeoutError): + if ( + sys.version_info >= (3, 11) + and (task := asyncio.current_task()) + and task.cancelling() + ): + raise # Then cancel handler and wait - with suppress(asyncio.CancelledError, asyncio.TimeoutError): + try: async with ceil_timeout(timeout): if self._current_request is not None: self._current_request._cancel(asyncio.CancelledError()) if self._task_handler is not None and not self._task_handler.done(): await self._task_handler + except (asyncio.CancelledError, asyncio.TimeoutError): + if ( + sys.version_info >= (3, 11) + and (task := asyncio.current_task()) + and task.cancelling() + ): + raise # force-close non-idle handler if self._task_handler is not None: @@ -523,8 +537,6 @@ async def start(self) -> None: # wait for next request self._waiter = loop.create_future() await self._waiter - except asyncio.CancelledError: - break finally: self._waiter = None @@ -551,7 +563,7 @@ async def start(self) -> None: task = loop.create_task(coro) try: resp, reset = await task - except (asyncio.CancelledError, ConnectionError): + except ConnectionError: self.log_debug("Ignored premature client disconnection") break @@ -577,12 +589,19 @@ async def start(self) -> None: now = loop.time() end_t = now + lingering_time - with suppress(asyncio.TimeoutError, asyncio.CancelledError): + try: while not payload.is_eof() and now < end_t: async with ceil_timeout(end_t - now): # read and ignore await payload.readany() now = loop.time() + except (asyncio.CancelledError, asyncio.TimeoutError): + if ( + sys.version_info >= (3, 11) + and (task := asyncio.current_task()) + and task.cancelling() + ): + raise # if payload still uncompleted if not payload.is_eof() and not self._force_close: @@ -590,10 +609,6 @@ async def start(self) -> None: self.close() set_exception(payload, PayloadAccessError()) - - except asyncio.CancelledError: - self.log_debug("Ignored premature client disconnection ") - break except Exception as exc: self.log_exception("Unhandled exception", exc_info=exc) self.force_close() diff --git a/tests/test_client_request.py b/tests/test_client_request.py index ecd02895e94..3eec24766f7 100644 --- a/tests/test_client_request.py +++ b/tests/test_client_request.py @@ -1234,6 +1234,24 @@ async def test_oserror_on_write_bytes( assert isinstance(exc, aiohttp.ClientOSError) +@pytest.mark.skipif(sys.version_info < (3, 11), reason="Needs Task.cancelling()") +async def test_cancel_close( + loop: asyncio.AbstractEventLoop, conn: mock.Mock +) -> None: + req = ClientRequest("get", URL("http://python.org"), loop=loop) + req._writer = asyncio.Future() + + t = asyncio.create_task(req.close()) + + # Start waiting on _writer + await asyncio.sleep(0) + + t.cancel() + # Cancellation should not be suppressed. + with pytest.raises(asyncio.CancelledError): + await t + + async def test_terminate(loop: asyncio.AbstractEventLoop, conn: mock.Mock) -> None: req = ClientRequest("get", URL("http://python.org"), loop=loop) diff --git a/tests/test_web_functional.py b/tests/test_web_functional.py index ec9279ccbf1..dd7648d837d 100644 --- a/tests/test_web_functional.py +++ b/tests/test_web_functional.py @@ -188,6 +188,43 @@ async def handler(request): resp.release() +@pytest.mark.skipif(sys.version_info < (3, 11), reason="Needs Task.cancelling()") +async def test_cancel_shutdown(aiohttp_client: Any) -> None: + async def handler(request): + t = asyncio.create_task(request.protocol.shutdown()) + # Ensure it's started waiting + await asyncio.sleep(0) + + t.cancel() + # Cancellation should not be suppressed + with pytest.raises(asyncio.CancelledError): + await t + + + # Repeat for second waiter in shutdown() + t = asyncio.create_task(request.protocol.shutdown()) + fut = asyncio.Future() + fut.set_result(None) + with mock.patch.object(request.protocol, "_handler_waiter", fut): + with mock.patch.object(request.protocol._current_request, "_cancel", autospec=True, spec_set=True): + await asyncio.sleep(0) + + t.cancel() + with pytest.raises(asyncio.CancelledError): + await t + + return web.Response(body=b"OK") + + app = web.Application() + app.router.add_get("/", handler) + client = await aiohttp_client(app) + + async with client.get("/") as resp: + assert resp.status == 200 + txt = await resp.text() + assert txt == "OK" + + async def test_post_form(aiohttp_client: Any) -> None: async def handler(request): data = await request.post() From 8d7b9d5f04735f7fecf1d689130325c378b1b9ec Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 5 Sep 2024 17:00:25 +0000 Subject: [PATCH 02/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_client_request.py | 4 +--- tests/test_web_functional.py | 8 ++++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/test_client_request.py b/tests/test_client_request.py index 3eec24766f7..801609d5b14 100644 --- a/tests/test_client_request.py +++ b/tests/test_client_request.py @@ -1235,9 +1235,7 @@ async def test_oserror_on_write_bytes( @pytest.mark.skipif(sys.version_info < (3, 11), reason="Needs Task.cancelling()") -async def test_cancel_close( - loop: asyncio.AbstractEventLoop, conn: mock.Mock -) -> None: +async def test_cancel_close(loop: asyncio.AbstractEventLoop, conn: mock.Mock) -> None: req = ClientRequest("get", URL("http://python.org"), loop=loop) req._writer = asyncio.Future() diff --git a/tests/test_web_functional.py b/tests/test_web_functional.py index dd7648d837d..c7b53add6c5 100644 --- a/tests/test_web_functional.py +++ b/tests/test_web_functional.py @@ -200,13 +200,17 @@ async def handler(request): with pytest.raises(asyncio.CancelledError): await t - # Repeat for second waiter in shutdown() t = asyncio.create_task(request.protocol.shutdown()) fut = asyncio.Future() fut.set_result(None) with mock.patch.object(request.protocol, "_handler_waiter", fut): - with mock.patch.object(request.protocol._current_request, "_cancel", autospec=True, spec_set=True): + with mock.patch.object( + request.protocol._current_request, + "_cancel", + autospec=True, + spec_set=True, + ): await asyncio.sleep(0) t.cancel() From 365b352759327bff58ace025ceeb517141941928 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 5 Sep 2024 18:01:35 +0100 Subject: [PATCH 03/15] Create 9030.bugfix.rst --- CHANGES/9030.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 CHANGES/9030.bugfix.rst diff --git a/CHANGES/9030.bugfix.rst b/CHANGES/9030.bugfix.rst new file mode 100644 index 00000000000..2e9d48f5359 --- /dev/null +++ b/CHANGES/9030.bugfix.rst @@ -0,0 +1 @@ +Fixed (on Python 3.11+) some edge cases where a task cancellation may get incorrectly suppressed -- by :user:`Dreamsorcerer`. From ab4211d16c964499cd3d213c16ed635d212751e4 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 5 Sep 2024 18:19:55 +0100 Subject: [PATCH 04/15] Update test_client_request.py --- tests/test_client_request.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_client_request.py b/tests/test_client_request.py index 801609d5b14..d575c07f8da 100644 --- a/tests/test_client_request.py +++ b/tests/test_client_request.py @@ -2,6 +2,7 @@ import hashlib import io import pathlib +import sys import zlib from http.cookies import BaseCookie, Morsel, SimpleCookie from typing import Any, AsyncIterator, Callable, Dict, Iterator, List, Protocol From dd7b8ac388086e4961e85021bf193a09f738fc38 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 5 Sep 2024 18:20:14 +0100 Subject: [PATCH 05/15] Update test_web_functional.py --- tests/test_web_functional.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_web_functional.py b/tests/test_web_functional.py index c7b53add6c5..43817ff339e 100644 --- a/tests/test_web_functional.py +++ b/tests/test_web_functional.py @@ -4,6 +4,7 @@ import json import pathlib import socket +import sys import zlib from typing import Any, NoReturn, Optional from unittest import mock From e65db9719c14e7005edcbaafc902b2a5ad019b30 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 5 Sep 2024 18:54:58 +0100 Subject: [PATCH 06/15] Fixes --- aiohttp/client_reqrep.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index 817e596edea..8509bc022dd 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -1045,7 +1045,15 @@ def _release_connection(self) -> None: async def _wait_released(self) -> None: if self._writer is not None: - await self._writer + try: + await self._writer + except asyncio.CancelledError: + if ( + sys.version_info >= (3, 11) + and (task := asyncio.current_task()) + and task.cancelling() + ): + raise self._release_connection() def _cleanup_writer(self) -> None: @@ -1062,7 +1070,15 @@ def _notify_content(self) -> None: async def wait_for_close(self) -> None: if self._writer is not None: - await self._writer + try: + await self._writer + except asyncio.CancelledError: + if ( + sys.version_info >= (3, 11) + and (task := asyncio.current_task()) + and task.cancelling() + ): + raise self.release() async def read(self) -> bytes: From 339b8ea24928620d7c29137043bb3a3d038b71cb Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 5 Sep 2024 19:14:47 +0100 Subject: [PATCH 07/15] Fixes --- aiohttp/web_protocol.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index 759b0447e75..b4ff97b4901 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -609,6 +609,9 @@ async def start(self) -> None: self.close() set_exception(payload, PayloadAccessError()) + except asyncio.CancelledError: + self.log_debug("Ignored premature client disconnection") + raise except Exception as exc: self.log_exception("Unhandled exception", exc_info=exc) self.force_close() From 96d5c7c14258e9c58db988fb7296c58915c637c3 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 5 Sep 2024 19:26:06 +0100 Subject: [PATCH 08/15] Fix --- tests/test_web_functional.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_web_functional.py b/tests/test_web_functional.py index 43817ff339e..5f5c0d8a7fb 100644 --- a/tests/test_web_functional.py +++ b/tests/test_web_functional.py @@ -207,8 +207,7 @@ async def handler(request): fut.set_result(None) with mock.patch.object(request.protocol, "_handler_waiter", fut): with mock.patch.object( - request.protocol._current_request, - "_cancel", + request.protocol, "_current_request", autospec=True, spec_set=True, ): From bb0c67d96b762aadd99f9a5bf77cdae1f78d8a66 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 5 Sep 2024 18:26:41 +0000 Subject: [PATCH 09/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_web_functional.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_web_functional.py b/tests/test_web_functional.py index 5f5c0d8a7fb..9d7deb10d15 100644 --- a/tests/test_web_functional.py +++ b/tests/test_web_functional.py @@ -207,7 +207,8 @@ async def handler(request): fut.set_result(None) with mock.patch.object(request.protocol, "_handler_waiter", fut): with mock.patch.object( - request.protocol, "_current_request", + request.protocol, + "_current_request", autospec=True, spec_set=True, ): From 57933dac0f01778df584bcef8f6962545eb0a27a Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 5 Sep 2024 19:28:44 +0100 Subject: [PATCH 10/15] Typing --- aiohttp/web_protocol.py | 4 ++-- tests/test_client_request.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index b4ff97b4901..987349d8bce 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -598,8 +598,8 @@ async def start(self) -> None: except (asyncio.CancelledError, asyncio.TimeoutError): if ( sys.version_info >= (3, 11) - and (task := asyncio.current_task()) - and task.cancelling() + and (t := asyncio.current_task()) + and t.cancelling() ): raise diff --git a/tests/test_client_request.py b/tests/test_client_request.py index d575c07f8da..4ba08e96c8b 100644 --- a/tests/test_client_request.py +++ b/tests/test_client_request.py @@ -1238,7 +1238,7 @@ async def test_oserror_on_write_bytes( @pytest.mark.skipif(sys.version_info < (3, 11), reason="Needs Task.cancelling()") async def test_cancel_close(loop: asyncio.AbstractEventLoop, conn: mock.Mock) -> None: req = ClientRequest("get", URL("http://python.org"), loop=loop) - req._writer = asyncio.Future() + req._writer = asyncio.Future() # type: ignore[assignment] t = asyncio.create_task(req.close()) From 7beaaebb55e35819a489fa1e446a89e7bd4e3ab9 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 5 Sep 2024 20:08:05 +0100 Subject: [PATCH 11/15] Tweak test --- tests/test_web_functional.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/test_web_functional.py b/tests/test_web_functional.py index 9d7deb10d15..b4868a81fad 100644 --- a/tests/test_web_functional.py +++ b/tests/test_web_functional.py @@ -202,16 +202,14 @@ async def handler(request): await t # Repeat for second waiter in shutdown() - t = asyncio.create_task(request.protocol.shutdown()) - fut = asyncio.Future() - fut.set_result(None) - with mock.patch.object(request.protocol, "_handler_waiter", fut): + with mock.patch.object(request.protocol, "_handler_waiter", None): with mock.patch.object( request.protocol, "_current_request", autospec=True, spec_set=True, ): + t = asyncio.create_task(request.protocol.shutdown()) await asyncio.sleep(0) t.cancel() From 17e87241b92a0509fe9a00b595f182c6db14f229 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sun, 22 Sep 2024 20:46:09 +0100 Subject: [PATCH 12/15] Update tests/test_web_functional.py --- tests/test_web_functional.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_web_functional.py b/tests/test_web_functional.py index 33f1477e36f..93c5dbde9ba 100644 --- a/tests/test_web_functional.py +++ b/tests/test_web_functional.py @@ -209,7 +209,7 @@ async def handler(request: web.Request) -> web.Response: await t # Repeat for second waiter in shutdown() - with mock.patch.object(request.protocol, "_handler_waiter", None): + with mock.patch.object(request.protocol, "_request_in_progress", False): with mock.patch.object( request.protocol, "_current_request", From 462a716be21581c14a437a57d7e819f6a4ff559a Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sun, 22 Sep 2024 21:03:42 +0100 Subject: [PATCH 13/15] Update aiohttp/web_protocol.py --- aiohttp/web_protocol.py | 1 + 1 file changed, 1 insertion(+) diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index 9e735010a98..301758545d6 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -295,6 +295,7 @@ async def shutdown(self, timeout: Optional[float] = 15.0) -> None: async with ceil_timeout(timeout): await self._handler_waiter except (asyncio.CancelledError, asyncio.TimeoutError): + self._handler_waiter = None if ( sys.version_info >= (3, 11) and (task := asyncio.current_task()) From d925a223a882710bcd773c69b60a7d2941447b8e Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sun, 22 Sep 2024 21:55:25 +0100 Subject: [PATCH 14/15] Update tests/test_web_functional.py --- tests/test_web_functional.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/test_web_functional.py b/tests/test_web_functional.py index 93c5dbde9ba..39337113704 100644 --- a/tests/test_web_functional.py +++ b/tests/test_web_functional.py @@ -210,12 +210,7 @@ async def handler(request: web.Request) -> web.Response: # Repeat for second waiter in shutdown() with mock.patch.object(request.protocol, "_request_in_progress", False): - with mock.patch.object( - request.protocol, - "_current_request", - autospec=True, - spec_set=True, - ): + with mock.patch.object(request.protocol, "_current_request", None): t = asyncio.create_task(request.protocol.shutdown()) await asyncio.sleep(0) From c9b9f1ddd0ebf69c2284149e779d183a7ee99757 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sun, 22 Sep 2024 22:58:29 +0100 Subject: [PATCH 15/15] Update aiohttp/web_protocol.py --- aiohttp/web_protocol.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index 301758545d6..15b5a829746 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -309,7 +309,7 @@ async def shutdown(self, timeout: Optional[float] = 15.0) -> None: self._current_request._cancel(asyncio.CancelledError()) if self._task_handler is not None and not self._task_handler.done(): - await self._task_handler + await asyncio.shield(self._task_handler) except (asyncio.CancelledError, asyncio.TimeoutError): if ( sys.version_info >= (3, 11)