From ba1b7b92e71f44b3d3328e9e633568ec41aab06b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 14 Nov 2024 13:28:23 -0600 Subject: [PATCH 01/12] Fix improperly closed WebSocket connections generating a backtrace We should only be generating a backtrace if loop.debug() is true when these get cleaned up by `__del__` in `ClientResponse` --- aiohttp/_websocket/reader_py.py | 3 +++ tests/test_client_ws_functional.py | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/aiohttp/_websocket/reader_py.py b/aiohttp/_websocket/reader_py.py index 04d654f8bf3..aa4b6ba2704 100644 --- a/aiohttp/_websocket/reader_py.py +++ b/aiohttp/_websocket/reader_py.py @@ -69,6 +69,9 @@ def __init__( self._get_buffer = self._buffer.popleft self._put_buffer = self._buffer.append + def is_eof(self) -> bool: + return self._eof + def exception(self) -> Optional[Union[Type[BaseException], BaseException]]: return self._exception diff --git a/tests/test_client_ws_functional.py b/tests/test_client_ws_functional.py index bcb1669c3e5..3889c3d6243 100644 --- a/tests/test_client_ws_functional.py +++ b/tests/test_client_ws_functional.py @@ -1232,3 +1232,27 @@ async def test_ws_connect_with_wrong_ssl_type(aiohttp_client: AiohttpClient) -> with pytest.raises(TypeError, match="ssl should be SSLContext, .*"): await session.ws_connect("/", ssl=42) + + +async def test_websocket_connection_not_closed_property( + aiohttp_client: AiohttpClient, +) -> None: + """Test that closing the connection via __del__ does not raise an exception.""" + + async def handler(request: web.Request) -> web.WebSocketResponse: + ws = web.WebSocketResponse() + await ws.prepare(request) + await ws.close() + return ws + + app = web.Application() + app.router.add_route("GET", "/", handler) + client = await aiohttp_client(app) + resp = await client.ws_connect("/") + assert resp._conn is not None + # Simulate the connection not being closed properly + # https://github.com/aio-libs/aiohttp/issues/9880 + resp._conn.release() + + # Clean up so the test does not leak + await resp.close() From 03a2af928ca33f8d2bd0d90e462d1280616a74d2 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 14 Nov 2024 13:29:23 -0600 Subject: [PATCH 02/12] changelog --- CHANGES/9883.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 CHANGES/9883.bugfix.rst diff --git a/CHANGES/9883.bugfix.rst b/CHANGES/9883.bugfix.rst new file mode 100644 index 00000000000..e42fb919984 --- /dev/null +++ b/CHANGES/9883.bugfix.rst @@ -0,0 +1 @@ +Fixed improperly closed WebSocket connections generating a backtrace -- by :user:`bdraco`. From 5e4d9be0ae81e8c578813d995909ba2666d7a3b9 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 14 Nov 2024 13:33:36 -0600 Subject: [PATCH 03/12] Update CHANGES/9883.bugfix.rst --- CHANGES/9883.bugfix.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES/9883.bugfix.rst b/CHANGES/9883.bugfix.rst index e42fb919984..3ffb8361448 100644 --- a/CHANGES/9883.bugfix.rst +++ b/CHANGES/9883.bugfix.rst @@ -1 +1 @@ -Fixed improperly closed WebSocket connections generating a backtrace -- by :user:`bdraco`. +Fixed improperly closed WebSocket connections generating an unhandled exception -- by :user:`bdraco`. From dac675ba0331c248430636b498fd9cf2d9795fc2 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 14 Nov 2024 13:34:20 -0600 Subject: [PATCH 04/12] cleanup --- tests/test_client_ws_functional.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_client_ws_functional.py b/tests/test_client_ws_functional.py index 3889c3d6243..4df3d89ba8d 100644 --- a/tests/test_client_ws_functional.py +++ b/tests/test_client_ws_functional.py @@ -1239,11 +1239,11 @@ async def test_websocket_connection_not_closed_property( ) -> None: """Test that closing the connection via __del__ does not raise an exception.""" - async def handler(request: web.Request) -> web.WebSocketResponse: + async def handler(request: web.Request) -> NoReturn: ws = web.WebSocketResponse() await ws.prepare(request) await ws.close() - return ws + assert False app = web.Application() app.router.add_route("GET", "/", handler) From 0c4361873971e128047de66f54640679d472412d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 14 Nov 2024 13:44:03 -0600 Subject: [PATCH 05/12] coverage --- tests/test_client_ws_functional.py | 36 +++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/tests/test_client_ws_functional.py b/tests/test_client_ws_functional.py index 4df3d89ba8d..b9190a36924 100644 --- a/tests/test_client_ws_functional.py +++ b/tests/test_client_ws_functional.py @@ -1,4 +1,5 @@ import asyncio +import gc import json import sys from typing import NoReturn, Optional @@ -1234,7 +1235,7 @@ async def test_ws_connect_with_wrong_ssl_type(aiohttp_client: AiohttpClient) -> await session.ws_connect("/", ssl=42) -async def test_websocket_connection_not_closed_property( +async def test_websocket_connection_not_closed_properly( aiohttp_client: AiohttpClient, ) -> None: """Test that closing the connection via __del__ does not raise an exception.""" @@ -1256,3 +1257,36 @@ async def handler(request: web.Request) -> NoReturn: # Clean up so the test does not leak await resp.close() + + +async def test_websocket_connection_cancellation( + aiohttp_client: AiohttpClient, loop: asyncio.AbstractEventLoop +) -> None: + """Test canceling the WebSocket connection task does not raise an exception in __del__.""" + + async def handler(request: web.Request) -> NoReturn: + ws = web.WebSocketResponse() + await ws.prepare(request) + await ws.close() + assert False + + sync_future: "asyncio.Future[None]" = loop.create_future() + + async def websocket_task() -> None: + app = web.Application() + app.router.add_route("GET", "/", handler) + client = await aiohttp_client(app) + resp = await client.ws_connect("/") + sync_future.set_result(None) + await asyncio.sleep(0) + await resp.close() + + task = loop.create_task(websocket_task()) + await sync_future + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + await asyncio.sleep(0) + + # Ensure any __del__ methods are called + gc.collect() From 81e77decc03b503293d8f7e83365b340b03909aa Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 14 Nov 2024 14:06:08 -0600 Subject: [PATCH 06/12] avoid gc --- tests/test_client_ws_functional.py | 33 ++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/tests/test_client_ws_functional.py b/tests/test_client_ws_functional.py index b9190a36924..9b5ff00bf9a 100644 --- a/tests/test_client_ws_functional.py +++ b/tests/test_client_ws_functional.py @@ -1,8 +1,7 @@ import asyncio -import gc import json import sys -from typing import NoReturn, Optional +from typing import List, NoReturn, Optional from unittest import mock import pytest @@ -1270,23 +1269,35 @@ async def handler(request: web.Request) -> NoReturn: await ws.close() assert False - sync_future: "asyncio.Future[None]" = loop.create_future() + app = web.Application() + app.router.add_route("GET", "/", handler) + + sync_future: "asyncio.Future[List[aiohttp.ClientWebSocketResponse]]" = ( + loop.create_future() + ) + client = await aiohttp_client(app) async def websocket_task() -> None: - app = web.Application() - app.router.add_route("GET", "/", handler) - client = await aiohttp_client(app) resp = await client.ws_connect("/") - sync_future.set_result(None) + # The test harness will cleanup the unclosed websocket + # for us, so we need to copy the websockets to ensure + # we can control the cleanup + sync_future.set_result(client._websockets.copy()) + client._websockets.clear() await asyncio.sleep(0) await resp.close() task = loop.create_task(websocket_task()) - await sync_future + websockets = await sync_future task.cancel() with pytest.raises(asyncio.CancelledError): await task - await asyncio.sleep(0) - # Ensure any __del__ methods are called - gc.collect() + websocket = websockets[0] + websockets.clear() + # Ensure any __del__ methods is called + del websocket._response + + # Cleanup properly + websocket._response = mock.Mock() + await websocket.close() From 2f8432aa627a8ac85537753e18723ce29cb6010e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 14 Nov 2024 14:06:57 -0600 Subject: [PATCH 07/12] avoid gc --- tests/test_client_ws_functional.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_client_ws_functional.py b/tests/test_client_ws_functional.py index 9b5ff00bf9a..d8496437a24 100644 --- a/tests/test_client_ws_functional.py +++ b/tests/test_client_ws_functional.py @@ -1296,6 +1296,8 @@ async def websocket_task() -> None: websocket = websockets[0] websockets.clear() # Ensure any __del__ methods is called + # since when it gets gc'd it not + # reproducible del websocket._response # Cleanup properly From be5575ac7ef64fbb8d3d6268c9d3aeef9804e203 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 14 Nov 2024 14:09:15 -0600 Subject: [PATCH 08/12] preen --- tests/test_client_ws_functional.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_client_ws_functional.py b/tests/test_client_ws_functional.py index d8496437a24..9b50fe82e9e 100644 --- a/tests/test_client_ws_functional.py +++ b/tests/test_client_ws_functional.py @@ -1293,8 +1293,7 @@ async def websocket_task() -> None: with pytest.raises(asyncio.CancelledError): await task - websocket = websockets[0] - websockets.clear() + websocket = websockets.pop() # Ensure any __del__ methods is called # since when it gets gc'd it not # reproducible From 3afb28520676617b4c49e90ccad6d5532067a2a5 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 14 Nov 2024 14:12:13 -0600 Subject: [PATCH 09/12] Update tests/test_client_ws_functional.py --- tests/test_client_ws_functional.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_client_ws_functional.py b/tests/test_client_ws_functional.py index 9b50fe82e9e..569c4abcade 100644 --- a/tests/test_client_ws_functional.py +++ b/tests/test_client_ws_functional.py @@ -1294,9 +1294,7 @@ async def websocket_task() -> None: await task websocket = websockets.pop() - # Ensure any __del__ methods is called - # since when it gets gc'd it not - # reproducible + # Call the `__del__` methods manually since when it gets gc'd it not reproducible del websocket._response # Cleanup properly From 6e2f17343c01c32383b1cd3159a39dd266806c04 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 14 Nov 2024 14:19:19 -0600 Subject: [PATCH 10/12] fix unreachable --- tests/test_client_ws_functional.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_client_ws_functional.py b/tests/test_client_ws_functional.py index 9b50fe82e9e..b56cdc57f24 100644 --- a/tests/test_client_ws_functional.py +++ b/tests/test_client_ws_functional.py @@ -1279,13 +1279,13 @@ async def handler(request: web.Request) -> NoReturn: async def websocket_task() -> None: resp = await client.ws_connect("/") + assert resp # The test harness will cleanup the unclosed websocket # for us, so we need to copy the websockets to ensure # we can control the cleanup sync_future.set_result(client._websockets.copy()) client._websockets.clear() await asyncio.sleep(0) - await resp.close() task = loop.create_task(websocket_task()) websockets = await sync_future From 97ff220e5471163a2ee3b7986be685f3e2db4aba Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 14 Nov 2024 14:21:40 -0600 Subject: [PATCH 11/12] preen --- tests/test_client_ws_functional.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_client_ws_functional.py b/tests/test_client_ws_functional.py index 65355bc2398..88930203608 100644 --- a/tests/test_client_ws_functional.py +++ b/tests/test_client_ws_functional.py @@ -1279,7 +1279,7 @@ async def handler(request: web.Request) -> NoReturn: async def websocket_task() -> None: resp = await client.ws_connect("/") - assert resp + assert resp is not None # ensure we hold a reference to the websocket # The test harness will cleanup the unclosed websocket # for us, so we need to copy the websockets to ensure # we can control the cleanup From 99f50953f21931aa7d5eabdb67b22805286dfe37 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 14 Nov 2024 14:21:55 -0600 Subject: [PATCH 12/12] preen --- tests/test_client_ws_functional.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_client_ws_functional.py b/tests/test_client_ws_functional.py index 88930203608..d42582f0078 100644 --- a/tests/test_client_ws_functional.py +++ b/tests/test_client_ws_functional.py @@ -1279,7 +1279,7 @@ async def handler(request: web.Request) -> NoReturn: async def websocket_task() -> None: resp = await client.ws_connect("/") - assert resp is not None # ensure we hold a reference to the websocket + assert resp is not None # ensure we hold a reference to the response # The test harness will cleanup the unclosed websocket # for us, so we need to copy the websockets to ensure # we can control the cleanup