Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix improperly closed WebSocket connections generating a backtrace #9883

Merged
merged 13 commits into from
Nov 14, 2024
1 change: 1 addition & 0 deletions CHANGES/9883.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed improperly closed WebSocket connections generating an unhandled exception -- by :user:`bdraco`.
3 changes: 3 additions & 0 deletions aiohttp/_websocket/reader_py.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
73 changes: 72 additions & 1 deletion tests/test_client_ws_functional.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import asyncio
import json
import sys
from typing import NoReturn, Optional
from typing import List, NoReturn, Optional
Dismissed Show dismissed Hide dismissed
from unittest import mock

import pytest
Expand Down Expand Up @@ -1232,3 +1232,74 @@

with pytest.raises(TypeError, match="ssl should be SSLContext, .*"):
await session.ws_connect("/", ssl=42)


async def test_websocket_connection_not_closed_properly(
aiohttp_client: AiohttpClient,
) -> None:
"""Test that closing the connection via __del__ does not raise an exception."""

async def handler(request: web.Request) -> NoReturn:
ws = web.WebSocketResponse()
await ws.prepare(request)
await ws.close()
assert False

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()
bdraco marked this conversation as resolved.
Show resolved Hide resolved

# 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

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:
resp = await client.ws_connect("/")
# 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
task.cancel()
with pytest.raises(asyncio.CancelledError):
await task

websocket = websockets[0]
websockets.clear()
# Ensure any __del__ methods is called
# since when it gets gc'd it not
# reproducible
bdraco marked this conversation as resolved.
Show resolved Hide resolved
del websocket._response

# Cleanup properly
websocket._response = mock.Mock()
await websocket.close()
Loading