From 8f04a5ca70a2c00394127fa96140711a58510347 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 17 Jun 2024 12:12:15 +0200 Subject: [PATCH 1/5] * Drops python3.7 support. It still works afaik, but is a pain to test in CI. * Adds loose mypy run to Makefile & CI * Adds some type annotations * Adds 3.13-dev test run. It currently fails, but will pass once trio and cffi push new releases. * Adds blurb to readme on status of the project. * Bump package versions in requirements-dev.txt and requirements-dev-full.txt * Adds small tests to slightly bump code coverage --- .github/workflows/ci.yml | 7 +- Makefile | 5 +- README.md | 4 + requirements-dev-full.txt | 151 +++++++++++++++++++++----------------- requirements-dev.txt | 51 +++++++------ requirements-extras.in | 1 + setup.py | 3 +- tests/test_connection.py | 20 ++++- trio_websocket/_impl.py | 57 +++++++++----- 9 files changed, 181 insertions(+), 118 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3e88af2..84ea2de 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,7 +14,7 @@ jobs: os: [ubuntu-latest] # latest pylint/astroid doesn't support 3.7 # python3.7 is covered in build_and_test_old_deps - python-version: ['3.8', '3.9', '3.10', '3.11', '3.12-dev'] + python-version: ['3.8', '3.9', '3.10', '3.11', '3.12'] steps: - uses: actions/checkout@v3 @@ -27,13 +27,14 @@ jobs: - run: pip install . -r requirements-dev-full.txt - run: make test - run: make lint + - run: make typecheck build_and_test_old_deps: runs-on: ${{ matrix.os }} strategy: matrix: os: [ubuntu-latest] - python-version: ['3.7'] + python-version: ['3.8'] steps: - uses: actions/checkout@v3 @@ -73,7 +74,7 @@ jobs: strategy: matrix: os: [ubuntu-latest] - python-version: ['3.12-dev'] + python-version: ['3.13-dev'] steps: - uses: actions/checkout@v3 - name: Setup Python diff --git a/Makefile b/Makefile index 2efced1..f97c321 100644 --- a/Makefile +++ b/Makefile @@ -8,11 +8,14 @@ docs: $(MAKE) -C docs html test: - $(PYTHON) -m pytest --cov=trio_websocket --no-cov-on-fail + $(PYTHON) -m pytest --cov=trio_websocket --cov-report=term-missing --no-cov-on-fail lint: $(PYTHON) -m pylint trio_websocket/ tests/ autobahn/ examples/ +typecheck: + $(PYTHON) -m mypy --explicit-package-bases trio_websocket tests autobahn examples + publish: rm -fr build dist .egg trio_websocket.egg-info ! grep -q dev trio_websocket/_version.py diff --git a/README.md b/README.md index 4b96dae..139f181 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,10 @@ available here](https://trio-websocket.readthedocs.io). ![Python Versions](https://img.shields.io/pypi/pyversions/trio-websocket.svg?style=flat-square) [![Build Status](https://img.shields.io/github/actions/workflow/status/python-trio/trio-websocket/ci.yml)](https://github.com/python-trio/trio-websocket/actions/workflows/ci.yml) [![Read the Docs](https://img.shields.io/readthedocs/trio-websocket.svg)](https://trio-websocket.readthedocs.io) +[![Join chatroom](https://img.shields.io/badge/chat-join%20now-blue.svg)](https://gitter.im/python-trio/general) + +## Status +**This project is on life-support maintenance.** If you're interested in helping to maintain and contribute, please reach out on [Gitter](https://gitter.im/python-trio/general) or contribute in issues and pull requests. ## Alternatives diff --git a/requirements-dev-full.txt b/requirements-dev-full.txt index 6ad3f76..a55a487 100644 --- a/requirements-dev-full.txt +++ b/requirements-dev-full.txt @@ -6,155 +6,175 @@ # alabaster==0.7.13 # via sphinx -astroid==3.0.0 +astroid==3.2.2 # via pylint -async-generator==1.10 - # via trio -attrs==22.2.0 +attrs==23.2.0 # via # -r requirements-dev.in # outcome # trio -babel==2.12.1 +babel==2.15.0 # via sphinx -bleach==6.0.0 - # via readme-renderer -build==0.10.0 +backports-tarfile==1.2.0 + # via jaraco-context +build==1.2.1 # via pip-tools -certifi==2022.12.7 +certifi==2024.6.2 # via requests -cffi==1.15.1 +cffi==1.16.0 # via cryptography -charset-normalizer==3.1.0 +charset-normalizer==3.3.2 # via requests -click==8.1.3 +click==8.1.7 # via pip-tools -coverage[toml]==7.2.1 +coverage[toml]==7.5.3 # via pytest-cov -cryptography==39.0.2 - # via trustme -dill==0.3.7 +cryptography==42.0.8 + # via + # secretstorage + # trustme +dill==0.3.8 # via pylint -docutils==0.18.1 +docutils==0.20.1 # via # readme-renderer # sphinx # sphinx-rtd-theme -exceptiongroup==1.1.3 ; python_version < "3.11" +exceptiongroup==1.2.1 ; python_version < "3.11" # via # pytest # trio # trio-websocket (setup.py) h11==0.14.0 # via wsproto -idna==3.4 +idna==3.7 # via # requests # trio # trustme imagesize==1.4.1 # via sphinx -importlib-metadata==6.0.0 +importlib-metadata==7.1.0 # via + # build # keyring # sphinx # twine -importlib-resources==6.1.0 +importlib-resources==6.4.0 # via keyring iniconfig==2.0.0 # via pytest -isort==5.11.5 +isort==5.13.2 # via pylint -jaraco-classes==3.2.3 +jaraco-classes==3.4.0 + # via keyring +jaraco-context==5.3.0 # via keyring -jinja2==3.1.2 +jaraco-functools==4.0.1 + # via keyring +jeepney==0.8.0 + # via + # keyring + # secretstorage +jinja2==3.1.4 # via sphinx -keyring==23.13.1 +keyring==25.2.1 # via twine -markdown-it-py==2.2.0 +markdown-it-py==3.0.0 # via rich -markupsafe==2.1.2 +markupsafe==2.1.5 # via jinja2 mccabe==0.7.0 # via pylint mdurl==0.1.2 # via markdown-it-py -more-itertools==9.1.0 - # via jaraco-classes -outcome==1.2.0 +more-itertools==10.2.0 + # via + # jaraco-classes + # jaraco-functools +mypy==1.10.0 + # via -r requirements-extras.in +mypy-extensions==1.0.0 + # via mypy +nh3==0.2.17 + # via readme-renderer +outcome==1.3.0.post0 # via # pytest-trio # trio -packaging==23.0 +packaging==24.1 # via # build # pytest # sphinx -pip-tools==6.14.0 +pip-tools==7.4.1 # via -r requirements-dev.in -pkginfo==1.9.6 +pkginfo==1.11.1 # via twine -platformdirs==3.1.1 +platformdirs==4.2.2 # via pylint -pluggy==1.0.0 +pluggy==1.5.0 # via pytest -pycparser==2.21 +pycparser==2.22 # via cffi -pygments==2.14.0 +pygments==2.18.0 # via # readme-renderer # rich # sphinx -pylint==3.0.0a7 +pylint==3.2.3 # via -r requirements-extras.in -pyproject-hooks==1.0.0 - # via build -pytest==7.4.2 +pyproject-hooks==1.1.0 + # via + # build + # pip-tools +pytest==8.2.2 # via # -r requirements-dev.in # pytest-cov # pytest-trio -pytest-cov==4.0.0 +pytest-cov==5.0.0 # via -r requirements-dev.in pytest-trio==0.8.0 # via -r requirements-dev.in -pytz==2023.3.post1 +pytz==2024.1 # via babel -readme-renderer==37.3 +readme-renderer==43.0 # via twine -requests==2.28.2 +requests==2.32.3 # via # requests-toolbelt # sphinx # twine -requests-toolbelt==0.10.1 +requests-toolbelt==1.0.0 # via twine rfc3986==2.0.0 # via twine -rich==13.3.2 +rich==13.7.1 # via twine -six==1.16.0 - # via bleach -sniffio==1.3.0 +secretstorage==3.3.3 + # via keyring +sniffio==1.3.1 # via trio snowballstemmer==2.2.0 # via sphinx sortedcontainers==2.4.0 # via trio -sphinx==5.3.0 +sphinx==7.1.2 # via # -r requirements-extras.in # sphinx-rtd-theme + # sphinxcontrib-jquery # sphinxcontrib-trio -sphinx-rtd-theme==1.2.0 +sphinx-rtd-theme==2.0.0 # via -r requirements-extras.in -sphinxcontrib-applehelp==1.0.2 +sphinxcontrib-applehelp==1.0.4 # via sphinx sphinxcontrib-devhelp==1.0.2 # via sphinx -sphinxcontrib-htmlhelp==2.0.0 +sphinxcontrib-htmlhelp==2.0.1 # via sphinx -sphinxcontrib-jquery==2.0.0 +sphinxcontrib-jquery==4.1 # via sphinx-rtd-theme sphinxcontrib-jsmath==1.0.1 # via sphinx @@ -168,36 +188,35 @@ tomli==2.0.1 # via # build # coverage + # mypy # pip-tools # pylint - # pyproject-hooks # pytest -tomlkit==0.11.6 +tomlkit==0.12.5 # via pylint -trio==0.22.0 +trio==0.25.1 # via # pytest-trio # trio-websocket (setup.py) -trustme==0.9.0 +trustme==1.1.0 # via -r requirements-dev.in -twine==4.0.2 +twine==5.1.0 # via -r requirements-extras.in -typing-extensions==4.8.0 +typing-extensions==4.12.2 # via # astroid + # mypy # pylint # rich -urllib3==1.26.15 +urllib3==2.2.1 # via # requests # twine -webencodings==0.5.1 - # via bleach -wheel==0.38.4 +wheel==0.43.0 # via pip-tools wsproto==1.2.0 # via trio-websocket (setup.py) -zipp==3.15.0 +zipp==3.19.2 # via # importlib-metadata # importlib-resources diff --git a/requirements-dev.txt b/requirements-dev.txt index 4ad6a84..f28f625 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -4,62 +4,64 @@ # # pip-compile --output-file=requirements-dev.txt requirements-dev.in setup.py # -async-generator==1.10 - # via trio -attrs==22.2.0 +attrs==23.2.0 # via # -r requirements-dev.in # outcome # trio -build==0.10.0 +build==1.2.1 # via pip-tools -cffi==1.15.1 +cffi==1.16.0 # via cryptography -click==8.1.3 +click==8.1.7 # via pip-tools -coverage[toml]==7.2.1 +coverage[toml]==7.5.3 # via pytest-cov -cryptography==41.0.4 +cryptography==42.0.8 # via trustme -exceptiongroup==1.1.3 ; python_version < "3.11" +exceptiongroup==1.2.1 ; python_version < "3.11" # via # pytest # trio # trio-websocket (setup.py) h11==0.14.0 # via wsproto -idna==3.4 +idna==3.7 # via # trio # trustme +importlib-metadata==7.1.0 + # via build iniconfig==2.0.0 # via pytest -outcome==1.2.0 +outcome==1.3.0.post0 # via # pytest-trio # trio -packaging==23.0 +packaging==24.1 # via # build # pytest -pip-tools==6.14.0 +pip-tools==7.4.1 # via -r requirements-dev.in -pluggy==1.0.0 +pluggy==1.5.0 # via pytest -pycparser==2.21 +pycparser==2.22 # via cffi -pyproject-hooks==1.0.0 - # via build -pytest==7.4.2 +pyproject-hooks==1.1.0 + # via + # build + # pip-tools +pytest==8.2.2 # via # -r requirements-dev.in # pytest-cov # pytest-trio -pytest-cov==4.0.0 +pytest-cov==5.0.0 # via -r requirements-dev.in pytest-trio==0.8.0 # via -r requirements-dev.in -sniffio==1.3.0 +sniffio==1.3.1 # via trio sortedcontainers==2.4.0 # via trio @@ -68,18 +70,19 @@ tomli==2.0.1 # build # coverage # pip-tools - # pyproject-hooks # pytest -trio==0.22.0 +trio==0.25.1 # via # pytest-trio # trio-websocket (setup.py) -trustme==0.9.0 +trustme==1.1.0 # via -r requirements-dev.in -wheel==0.38.4 +wheel==0.43.0 # via pip-tools wsproto==1.2.0 # via trio-websocket (setup.py) +zipp==3.19.2 + # via importlib-metadata # The following packages are considered to be unsafe in a requirements file: # pip diff --git a/requirements-extras.in b/requirements-extras.in index 9f4d0c5..926b566 100644 --- a/requirements-extras.in +++ b/requirements-extras.in @@ -1,5 +1,6 @@ # requirements for `make lint/docs/publish` pylint +mypy sphinx sphinxcontrib-trio sphinx_rtd_theme diff --git a/setup.py b/setup.py index a5040a6..17a21f9 100644 --- a/setup.py +++ b/setup.py @@ -28,7 +28,6 @@ 'Intended Audience :: Developers', 'Topic :: Software Development :: Libraries', 'License :: OSI Approved :: MIT License', - 'Programming Language :: Python :: 3.7', 'Programming Language :: Python :: 3.8', 'Programming Language :: Python :: 3.9', 'Programming Language :: Python :: 3.10', @@ -37,7 +36,7 @@ 'Programming Language :: Python :: Implementation :: CPython', 'Programming Language :: Python :: Implementation :: PyPy', ], - python_requires=">=3.7", + python_requires=">=3.8", keywords='websocket client server trio', packages=find_packages(exclude=['docs', 'examples', 'tests']), install_requires=[ diff --git a/tests/test_connection.py b/tests/test_connection.py index 096a172..3d45eb7 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -29,6 +29,8 @@ the server to block until the client has sent the closing handshake. In other circumstances ''' +from __future__ import annotations + from functools import partial, wraps import ssl from unittest.mock import patch @@ -44,7 +46,7 @@ try: from trio.lowlevel import current_task # pylint: disable=ungrouped-imports except ImportError: - from trio.hazmat import current_task # pylint: disable=ungrouped-imports + from trio.hazmat import current_task # type: ignore # pylint: disable=ungrouped-imports from trio_websocket import ( connect_websocket, @@ -129,8 +131,10 @@ async def wrapper(*args, **kwargs): @attr.s(hash=False, eq=False) class MemoryListener(trio.abc.Listener): closed = attr.ib(default=False) - accepted_streams = attr.ib(factory=list) - queued_streams = attr.ib(factory=lambda: trio.open_memory_channel(1)) + accepted_streams: list[ + tuple[trio.abc.SendChannel[str], trio.abc.ReceiveChannel[str]] + ] = attr.ib(factory=list) + queued_streams = attr.ib(factory=lambda: trio.open_memory_channel[str](1)) accept_hook = attr.ib(default=None) async def connect(self): @@ -290,6 +294,14 @@ async def test_client_open_invalid_url(echo_server): async with open_websocket_url('http://foo.com/bar'): pass +async def test_client_open_invalid_ssl(echo_server, nursery): + with pytest.raises(TypeError, match='`use_ssl` argument must be bool or ssl.SSLContext'): + await connect_websocket(nursery, HOST, echo_server.port, RESOURCE, use_ssl=1) + + url = f'ws://{HOST}:{echo_server.port}{RESOURCE}' + with pytest.raises(ValueError, match='^SSL context must be None for ws: URL scheme$' ): + await connect_websocket_url(nursery, url, ssl_context=ssl.SSLContext(ssl.PROTOCOL_SSLv23)) + async def test_ascii_encoded_path_is_ok(echo_server): path = '%D7%90%D7%91%D7%90?%D7%90%D7%9E%D7%90' @@ -417,7 +429,7 @@ async def handler(request): @fail_after(1) -async def test_handshake_exception_before_accept(): +async def test_handshake_exception_before_accept() -> None: ''' In #107, a request handler that throws an exception before finishing the handshake causes the task to hang. The proper behavior is to raise an exception to the nursery as soon as possible. ''' diff --git a/trio_websocket/_impl.py b/trio_websocket/_impl.py index 259f4fd..b153034 100644 --- a/trio_websocket/_impl.py +++ b/trio_websocket/_impl.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import sys from collections import OrderedDict from contextlib import asynccontextmanager @@ -9,7 +11,7 @@ import ssl import struct import urllib.parse -from typing import List, Optional, Union +from typing import Iterable, List, Optional, Union import trio import trio.abc @@ -70,7 +72,7 @@ def __exit__(self, ty, value, tb): if _TRIO_MULTI_ERROR: # pragma: no cover filtered_exception = trio.MultiError.filter(_ignore_cancel, value) # pylint: disable=no-member - elif isinstance(value, BaseExceptionGroup): + elif isinstance(value, BaseExceptionGroup): # pylint: disable=possibly-used-before-assignment filtered_exception = value.subgroup(lambda exc: not isinstance(exc, trio.Cancelled)) else: filtered_exception = _ignore_cancel(value) @@ -78,10 +80,19 @@ def __exit__(self, ty, value, tb): @asynccontextmanager -async def open_websocket(host, port, resource, *, use_ssl, subprotocols=None, - extra_headers=None, - message_queue_size=MESSAGE_QUEUE_SIZE, max_message_size=MAX_MESSAGE_SIZE, - connect_timeout=CONN_TIMEOUT, disconnect_timeout=CONN_TIMEOUT): +async def open_websocket( + host: str, + port: int, + resource: str, + *, + use_ssl: Union[bool, ssl.SSLContext], + subprotocols: Optional[Iterable[str]] = None, + extra_headers: Optional[list[tuple[bytes,bytes]]] = None, + message_queue_size: int = MESSAGE_QUEUE_SIZE, + max_message_size: int = MAX_MESSAGE_SIZE, + connect_timeout: float = CONN_TIMEOUT, + disconnect_timeout: float = CONN_TIMEOUT + ): ''' Open a WebSocket client connection to a host. @@ -138,7 +149,8 @@ async def open_websocket(host, port, resource, *, use_ssl, subprotocols=None, async def connect_websocket(nursery, host, port, resource, *, use_ssl, subprotocols=None, extra_headers=None, - message_queue_size=MESSAGE_QUEUE_SIZE, max_message_size=MAX_MESSAGE_SIZE): + message_queue_size=MESSAGE_QUEUE_SIZE, max_message_size=MAX_MESSAGE_SIZE + ) -> WebSocketConnection: ''' Return an open WebSocket client connection to a host. @@ -179,6 +191,7 @@ async def connect_websocket(nursery, host, port, resource, *, use_ssl, logger.debug('Connecting to ws%s://%s:%d%s', '' if ssl_context is None else 's', host, port, resource) + stream: trio.SSLStream[trio.SocketStream] | trio.SocketStream if ssl_context is None: stream = await trio.open_tcp_stream(host, port) else: @@ -684,10 +697,17 @@ class WebSocketConnection(trio.abc.AsyncResource): CONNECTION_ID = itertools.count() - def __init__(self, stream, ws_connection, *, host=None, path=None, - client_subprotocols=None, client_extra_headers=None, - message_queue_size=MESSAGE_QUEUE_SIZE, - max_message_size=MAX_MESSAGE_SIZE): + def __init__( + self, + stream: trio.SocketStream | trio.SSLStream[trio.SocketStream], + ws_connection: wsproto.WSConnection, + *, + host=None, + path=None, + client_subprotocols=None, client_extra_headers=None, + message_queue_size=MESSAGE_QUEUE_SIZE, + max_message_size=MAX_MESSAGE_SIZE + ): ''' Constructor. @@ -734,13 +754,14 @@ def __init__(self, stream, ws_connection, *, host=None, path=None, self._initial_request = None self._path = path self._subprotocol: Optional[str] = None - self._handshake_headers = tuple() + self._handshake_headers: tuple[tuple[str,str], ...] = tuple() self._reject_status = 0 - self._reject_headers = tuple() + self._reject_headers: tuple[tuple[str,str], ...] = tuple() self._reject_body = b'' - self._send_channel, self._recv_channel = trio.open_memory_channel( - message_queue_size) - self._pings = OrderedDict() + self._send_channel, self._recv_channel = trio.open_memory_channel[ + Union[bytes, str] + ](message_queue_size) + self._pings: OrderedDict[bytes, trio.Event] = OrderedDict() # Set when the server has received a connection request event. This # future is never set on client connections. self._connection_proposal = Future() @@ -892,7 +913,7 @@ async def get_message(self): raise ConnectionClosed(self._close_reason) from None return message - async def ping(self, payload=None): + async def ping(self, payload: bytes|None=None): ''' Send WebSocket ping to remote endpoint and wait for a correspoding pong. @@ -915,7 +936,7 @@ async def ping(self, payload=None): if self._close_reason: raise ConnectionClosed(self._close_reason) if payload in self._pings: - raise ValueError(f'Payload value {payload} is already in flight.') + raise ValueError(f'Payload value {payload!r} is already in flight.') if payload is None: payload = struct.pack('!I', random.getrandbits(32)) event = trio.Event() From 7eb779b727b4e029391db50a5f6e24b9541dece2 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Thu, 6 Jun 2024 18:04:25 +0200 Subject: [PATCH 2/5] handle strict_exception_groups=True by unwrapping user exceptions from within exceptiongroups revert making close_connection CS shielded, as that would be a behaviour change causing very long stalls with the default timeout of 60s add comment for pylint disable move RaisesGroup import --- tests/test_connection.py | 31 +++++++++++++++++- trio_websocket/_impl.py | 68 ++++++++++++++++++++++++++++++++++------ 2 files changed, 89 insertions(+), 10 deletions(-) diff --git a/tests/test_connection.py b/tests/test_connection.py index 3d45eb7..ab43725 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -32,7 +32,9 @@ from __future__ import annotations from functools import partial, wraps +import re import ssl +import sys from unittest.mock import patch import attr @@ -48,6 +50,13 @@ except ImportError: from trio.hazmat import current_task # type: ignore # pylint: disable=ungrouped-imports + +# only available on trio>=0.25, we don't use it when testing lower versions +try: + from trio.testing import RaisesGroup +except ImportError: + pass + from trio_websocket import ( connect_websocket, connect_websocket_url, @@ -66,6 +75,9 @@ wrap_server_stream ) +if sys.version_info < (3, 11): + from exceptiongroup import BaseExceptionGroup # pylint: disable=redefined-builtin + WS_PROTO_VERSION = tuple(map(int, wsproto.__version__.split('.'))) HOST = '127.0.0.1' @@ -427,6 +439,9 @@ async def handler(request): assert header_key == b'x-test-header' assert header_value == b'My test header' +def _trio_default_loose() -> bool: + assert re.match(r'^0\.\d\d\.', trio.__version__), "unexpected trio versioning scheme" + return int(trio.__version__[2:4]) < 25 @fail_after(1) async def test_handshake_exception_before_accept() -> None: @@ -436,7 +451,8 @@ async def test_handshake_exception_before_accept() -> None: async def handler(request): raise ValueError() - with pytest.raises(ValueError): + # pylint fails to resolve that BaseExceptionGroup will always be available + with pytest.raises((BaseExceptionGroup, ValueError)) as exc: # pylint: disable=possibly-used-before-assignment async with trio.open_nursery() as nursery: server = await nursery.start(serve_websocket, handler, HOST, 0, None) @@ -444,6 +460,19 @@ async def handler(request): use_ssl=False): pass + if _trio_default_loose(): + assert isinstance(exc.value, ValueError) + else: + # there's 4 levels of nurseries opened, leading to 4 nested groups: + # 1. this test + # 2. WebSocketServer.run + # 3. trio.serve_listeners + # 4. WebSocketServer._handle_connection + assert RaisesGroup( + RaisesGroup( + RaisesGroup( + RaisesGroup(ValueError)))).matches(exc.value) + @fail_after(1) async def test_reject_handshake(nursery): diff --git a/trio_websocket/_impl.py b/trio_websocket/_impl.py index b153034..f28eb15 100644 --- a/trio_websocket/_impl.py +++ b/trio_websocket/_impl.py @@ -13,6 +13,7 @@ import urllib.parse from typing import Iterable, List, Optional, Union +import outcome import trio import trio.abc from wsproto import ConnectionType, WSConnection @@ -44,6 +45,10 @@ logger = logging.getLogger('trio-websocket') +class TrioWebsocketInternalError(Exception): + ... + + def _ignore_cancel(exc): return None if isinstance(exc, trio.Cancelled) else exc @@ -125,10 +130,10 @@ async def open_websocket( client-side timeout (:exc:`ConnectionTimeout`, :exc:`DisconnectionTimeout`), or server rejection (:exc:`ConnectionRejected`) during handshakes. ''' - async with trio.open_nursery() as new_nursery: + async def open_connection(nursery: trio.Nursery) -> WebSocketConnection: try: with trio.fail_after(connect_timeout): - connection = await connect_websocket(new_nursery, host, port, + return await connect_websocket(nursery, host, port, resource, use_ssl=use_ssl, subprotocols=subprotocols, extra_headers=extra_headers, message_queue_size=message_queue_size, @@ -137,14 +142,59 @@ async def open_websocket( raise ConnectionTimeout from None except OSError as e: raise HandshakeError from e + + async def close_connection(connection: WebSocketConnection) -> None: try: - yield connection - finally: - try: - with trio.fail_after(disconnect_timeout): - await connection.aclose() - except trio.TooSlowError: - raise DisconnectionTimeout from None + with trio.fail_after(disconnect_timeout): + await connection.aclose() + except trio.TooSlowError: + raise DisconnectionTimeout from None + + connection: WebSocketConnection|None=None + result2: outcome.Maybe[None] | None = None + user_error = None + + try: + async with trio.open_nursery() as new_nursery: + result = await outcome.acapture(open_connection, new_nursery) + + if isinstance(result, outcome.Value): + connection = result.unwrap() + try: + yield connection + except BaseException as e: + user_error = e + raise + finally: + result2 = await outcome.acapture(close_connection, connection) + # This exception handler should only be entered if: + # 1. The _reader_task started in connect_websocket raises + # 2. User code raises an exception + except BaseExceptionGroup as e: + # user_error, or exception bubbling up from _reader_task + if len(e.exceptions) == 1: + raise e.exceptions[0] + # if the group contains two exceptions, one being Cancelled, and the other + # is user_error => drop Cancelled and raise user_error + # This Cancelled should only have been able to come from _reader_task + if ( + len(e.exceptions) == 2 + and user_error is not None + and user_error in e.exceptions + and any(isinstance(exc, trio.Cancelled) for exc in e.exceptions) + ): + raise user_error # pylint: disable=raise-missing-from,raising-bad-type + raise TrioWebsocketInternalError from e # pragma: no cover + ## TODO: handle keyboardinterrupt? + + finally: + if result2 is not None: + result2.unwrap() + + + # error setting up, unwrap that exception + if connection is None: + result.unwrap() async def connect_websocket(nursery, host, port, resource, *, use_ssl, From 8fd6db1d26ec6b3da01419dfee377da388a19e85 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 19 Jun 2024 15:47:54 +0200 Subject: [PATCH 3/5] revamp exception handling, add multiple tests that cover most plausible cases --- tests/test_connection.py | 83 ++++++++++++++++++++++++++++++++++++++++ trio_websocket/_impl.py | 76 +++++++++++++++++++++++++++++------- 2 files changed, 145 insertions(+), 14 deletions(-) diff --git a/tests/test_connection.py b/tests/test_connection.py index ab43725..b5d4100 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -69,6 +69,7 @@ open_websocket, open_websocket_url, serve_websocket, + WebSocketConnection, WebSocketServer, WebSocketRequest, wrap_client_stream, @@ -439,6 +440,88 @@ async def handler(request): assert header_key == b'x-test-header' assert header_value == b'My test header' + + + +@fail_after(5) +async def test_open_websocket_internal_ki(nursery, monkeypatch, autojump_clock): + """_reader_task._handle_ping_event triggers KeyboardInterrupt. + user code also raises exception. + Make sure that KI is delivered, and the user exception is in the __cause__ exceptiongroup + """ + async def ki_raising_ping_handler(*args, **kwargs) -> None: + print("raising ki") + raise KeyboardInterrupt + monkeypatch.setattr(WebSocketConnection, "_handle_ping_event", ki_raising_ping_handler) + async def handler(request): + server_ws = await request.accept() + await server_ws.ping(b"a") + + server = await nursery.start(serve_websocket, handler, HOST, 0, None) + with pytest.raises(KeyboardInterrupt) as exc_info: + async with open_websocket(HOST, server.port, RESOURCE, use_ssl=False): + with trio.fail_after(1) as cs: + cs.shield = True + await trio.sleep(2) + + e_cause = exc_info.value.__cause__ + assert isinstance(e_cause, BaseExceptionGroup) + assert any(isinstance(e, trio.TooSlowError) for e in e_cause.exceptions) + +@fail_after(5) +async def test_open_websocket_internal_exc(nursery, monkeypatch, autojump_clock): + """_reader_task._handle_ping_event triggers ValueError. + user code also raises exception. + internal exception is in __cause__ exceptiongroup and user exc is delivered + """ + my_value_error = ValueError() + async def raising_ping_event(*args, **kwargs) -> None: + raise my_value_error + + monkeypatch.setattr(WebSocketConnection, "_handle_ping_event", raising_ping_event) + async def handler(request): + server_ws = await request.accept() + await server_ws.ping(b"a") + + server = await nursery.start(serve_websocket, handler, HOST, 0, None) + with pytest.raises(trio.TooSlowError) as exc_info: + async with open_websocket(HOST, server.port, RESOURCE, use_ssl=False): + with trio.fail_after(1) as cs: + cs.shield = True + await trio.sleep(2) + + e_cause = exc_info.value.__cause__ + assert isinstance(e_cause, BaseExceptionGroup) + assert my_value_error in e_cause.exceptions + +@fail_after(5) +async def test_open_websocket_cancellations(nursery, monkeypatch, autojump_clock): + """Both user code and _reader_task raise Cancellation. + Check that open_websocket reraises the one from user code for traceback reasons. + + We monkeypatch WebSocketConnection._handle_ping_event to ensure it will actually + raise Cancelled upon being cancelled. For some reason it doesn't otherwise.""" + + async def sleeping_ping_event(*args, **kwargs) -> None: + await trio.sleep_forever() + + monkeypatch.setattr(WebSocketConnection, "_handle_ping_event", sleeping_ping_event) + async def handler(request): + server_ws = await request.accept() + await server_ws.ping(b"a") + user_cancelled = None + + server = await nursery.start(serve_websocket, handler, HOST, 0, None) + with trio.move_on_after(2): + with pytest.raises(trio.Cancelled) as exc_info: + async with open_websocket(HOST, server.port, RESOURCE, use_ssl=False): + try: + await trio.sleep_forever() + except trio.Cancelled as e: + user_cancelled = e + raise + assert exc_info.value is user_cancelled + def _trio_default_loose() -> bool: assert re.match(r'^0\.\d\d\.', trio.__version__), "unexpected trio versioning scheme" return int(trio.__version__[2:4]) < 25 diff --git a/trio_websocket/_impl.py b/trio_websocket/_impl.py index f28eb15..ebfaac6 100644 --- a/trio_websocket/_impl.py +++ b/trio_websocket/_impl.py @@ -46,7 +46,10 @@ class TrioWebsocketInternalError(Exception): - ... + """Raised as a fallback when open_websocket is unable to unwind an exceptiongroup + into a single preferred exception. This should never happen, if it does then + underlying assumptions about the internal code are incorrect. + """ def _ignore_cancel(exc): @@ -130,6 +133,29 @@ async def open_websocket( client-side timeout (:exc:`ConnectionTimeout`, :exc:`DisconnectionTimeout`), or server rejection (:exc:`ConnectionRejected`) during handshakes. ''' + + # This context manager tries very very hard not to raise an exceptiongroup + # in order to be as transparent as possible for the end user. + # In the trivial case, this means that if user code inside the cm raises + # we make sure that it doesn't get wrapped. + + # If opening the connection fails, then we will raise that exception. User + # code is never executed, so we will never have multiple exceptions. + + # After opening the connection, we spawn _reader_task in the background and + # yield to user code. If only one of those raise a non-cancelled exception + # we will raise that non-cancelled exception. + # If we get multiple cancelled, we raise the user's cancelled. + # If both raise exceptions, we raise the user code's exception with the entire + # exception group as the __cause__. + # If we somehow get multiple exceptions, but no user exception, then we raise + # TrioWebsocketInternalError. + + # If closing the connection fails, then that will be raised as the top + # exception in the last `finally`. If we encountered exceptions in user code + # or in reader task then they will be set as the `__cause__`. + + async def open_connection(nursery: trio.Nursery) -> WebSocketConnection: try: with trio.fail_after(connect_timeout): @@ -167,25 +193,47 @@ async def close_connection(connection: WebSocketConnection) -> None: raise finally: result2 = await outcome.acapture(close_connection, connection) - # This exception handler should only be entered if: + # This exception handler should only be entered if either: # 1. The _reader_task started in connect_websocket raises # 2. User code raises an exception + # I.e. open/close_connection are not included except BaseExceptionGroup as e: # user_error, or exception bubbling up from _reader_task if len(e.exceptions) == 1: raise e.exceptions[0] - # if the group contains two exceptions, one being Cancelled, and the other - # is user_error => drop Cancelled and raise user_error - # This Cancelled should only have been able to come from _reader_task - if ( - len(e.exceptions) == 2 - and user_error is not None - and user_error in e.exceptions - and any(isinstance(exc, trio.Cancelled) for exc in e.exceptions) - ): - raise user_error # pylint: disable=raise-missing-from,raising-bad-type - raise TrioWebsocketInternalError from e # pragma: no cover - ## TODO: handle keyboardinterrupt? + + # contains at most 1 non-cancelled exceptions + exception_to_raise: BaseException|None = None + for sub_exc in e.exceptions: + if not isinstance(sub_exc, trio.Cancelled): + if exception_to_raise is not None: + # multiple non-cancelled + break + exception_to_raise = sub_exc + else: + if exception_to_raise is None: + # all exceptions are cancelled + # prefer raising the one from the user, for traceback reasons + if user_error is not None: + raise user_error + # multiple internal Cancelled is not possible afaik + raise e.exceptions[0] # pragma: no cover + raise exception_to_raise + + # if we have any KeyboardInterrupt in the group, make sure to raise it. + for sub_exc in e.exceptions: + if isinstance(sub_exc, KeyboardInterrupt): + raise sub_exc from e + + # Both user code and internal code raised non-cancelled exceptions. + # We "hide" the internal exception(s) in the __cause__ and surface + # the user_error. + if user_error is not None: + raise user_error from e + + raise TrioWebsocketInternalError( + "Multiple internal exceptions should not be possible. Please report this as a bug to https://github.com/python-trio/trio-websocket" + ) from e # pragma: no cover finally: if result2 is not None: From 69853c34f6fdadb1051ded6bc1e0af4a18483d56 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 19 Jun 2024 16:06:47 +0200 Subject: [PATCH 4/5] fix pylint warnings --- trio_websocket/_impl.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/trio_websocket/_impl.py b/trio_websocket/_impl.py index ebfaac6..181850c 100644 --- a/trio_websocket/_impl.py +++ b/trio_websocket/_impl.py @@ -215,9 +215,11 @@ async def close_connection(connection: WebSocketConnection) -> None: # all exceptions are cancelled # prefer raising the one from the user, for traceback reasons if user_error is not None: - raise user_error + # no reason to raise from e, just to include a bunch of extra + # cancelleds. + raise user_error # pylint: disable=raise-missing-from # multiple internal Cancelled is not possible afaik - raise e.exceptions[0] # pragma: no cover + raise e.exceptions[0] # pragma: no cover # pylint: disable=raise-missing-from raise exception_to_raise # if we have any KeyboardInterrupt in the group, make sure to raise it. @@ -232,7 +234,9 @@ async def close_connection(connection: WebSocketConnection) -> None: raise user_error from e raise TrioWebsocketInternalError( - "Multiple internal exceptions should not be possible. Please report this as a bug to https://github.com/python-trio/trio-websocket" + "Multiple internal exceptions should not be possible. " + "Please report this as a bug to " + "https://github.com/python-trio/trio-websocket" ) from e # pragma: no cover finally: From f22fa758840d6744ed189acb921c84c478b3d32c Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 19 Jun 2024 16:27:51 +0200 Subject: [PATCH 5/5] fix old_deps --- tests/test_connection.py | 11 +++++++++-- trio_websocket/_impl.py | 7 ++++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/tests/test_connection.py b/tests/test_connection.py index b5d4100..7401554 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -76,9 +76,16 @@ wrap_server_stream ) +from trio_websocket._impl import _TRIO_MULTI_ERROR + if sys.version_info < (3, 11): from exceptiongroup import BaseExceptionGroup # pylint: disable=redefined-builtin +if _TRIO_MULTI_ERROR: + EXCEPTION_GROUP_TYPE = trio.MultiError # type: ignore[attr-defined] # pylint: disable=no-member +else: + EXCEPTION_GROUP_TYPE = BaseExceptionGroup + WS_PROTO_VERSION = tuple(map(int, wsproto.__version__.split('.'))) HOST = '127.0.0.1' @@ -465,7 +472,7 @@ async def handler(request): await trio.sleep(2) e_cause = exc_info.value.__cause__ - assert isinstance(e_cause, BaseExceptionGroup) + assert isinstance(e_cause, EXCEPTION_GROUP_TYPE) assert any(isinstance(e, trio.TooSlowError) for e in e_cause.exceptions) @fail_after(5) @@ -491,7 +498,7 @@ async def handler(request): await trio.sleep(2) e_cause = exc_info.value.__cause__ - assert isinstance(e_cause, BaseExceptionGroup) + assert isinstance(e_cause, EXCEPTION_GROUP_TYPE) assert my_value_error in e_cause.exceptions @fail_after(5) diff --git a/trio_websocket/_impl.py b/trio_websocket/_impl.py index 181850c..aba02ef 100644 --- a/trio_websocket/_impl.py +++ b/trio_websocket/_impl.py @@ -176,6 +176,11 @@ async def close_connection(connection: WebSocketConnection) -> None: except trio.TooSlowError: raise DisconnectionTimeout from None + if _TRIO_MULTI_ERROR: + exception_group_type = trio.MultiError # type: ignore[attr-defined] # pylint: disable=no-member + else: + exception_group_type = BaseExceptionGroup + connection: WebSocketConnection|None=None result2: outcome.Maybe[None] | None = None user_error = None @@ -197,7 +202,7 @@ async def close_connection(connection: WebSocketConnection) -> None: # 1. The _reader_task started in connect_websocket raises # 2. User code raises an exception # I.e. open/close_connection are not included - except BaseExceptionGroup as e: + except exception_group_type as e: # user_error, or exception bubbling up from _reader_task if len(e.exceptions) == 1: raise e.exceptions[0]