Support strict_exception_groups=True #188

jakkdl · 2024-06-06T16:29:57Z

Changes open_websocket to only raise a single exception, even when running under strict_exception_groups=True
- Should maybe introduce special handling for KeyboardInterrupts
- If multiple non-Cancelled-exceptions are encountered, then it will raise TrioWebSocketInternalError with the exceptiongroup as its __cause__. This should only be possible if the background task and the user context both raise exceptions. This would previously raise a MultiError with both Exceptions.
- other alternatives could include throwing out the exception from the background task, raising an ExceptionGroup with both errors, or trying to do something fancy with __cause__ or __context__.
WebSocketServer.run and WebSocketServer._handle_connection are the other two places that opens a nursery. I've opted not to change these, since I don't think user code should expect any special exceptions from it, and it seems less obscure that it might contain an internal nursery.
- Update docstrings to mention existence of internal nursery.

split out into #189

drops python3.7 from CI, and updates setup.py. The code almost surely still works on 3.7 though, but testing it is a pain.
adds a loose mypy run to CI to check the few annotations that are strewn throughout the code
- adds a couple annotations where mypy complained
3.13 currently fails, but once trio makes a new release and cffi==1.17.0rc1 is fully released, the build_and_test_latest run will pass
Adds a blurb to the readme saying this project is on life-support maintenance.
bumps requirements-dev.txt and requirements-dev-full.txt
adds some small tests to bump code coverage

belm0 · 2024-06-06T18:34:52Z

trio_websocket/_impl.py

Hi John-- I may be slow in reviewing.

Please make sure the PR description summarizes the changes (e.g. mentions the internal error, API policy about exception groups, etc.).

Would need test coverage for any changes, but ok to hold off until I've reviewed the basics (as I can't promise we'll take the direction of this PR).

I'm itching to add Black and type checking to the CI, but that should be in a separate PR.

Please see #177. On type checking, I'm ok running a loose mypy pass, but reluctant to convert the codebase to strict type annotations, since fighting the type system is often a burden on maintenance (especially with a project on life support).

Ah thank you, of course I'd complained about lack of Black before 😅

Agree on loose mypy just to check if the annotations that exist are correct. And I don't have any plans on adding annotations everywhere, just doing it for my own sake when developing.

Will try and make PR summary verbose and descriptive 👍

jakkdl · 2024-06-07T13:54:42Z

@belm0 do you have strong opinions on keeping python3.7 support? It's getting problematic to support with lotsa libraries not supporting it. If pinning test requirements to versions that support 3.7 we're not testing anything remotely recent, so would need to generate an additional requirements.txt file, etc etc.

Testing trio versions < 0.22 is also getting messy. If we explicitly want to check for strict_exception_group compatibility the parameter was only introduced in 0.22
I would also love to use trio.testing.RaisesGroup, but at that point the test deps hit trio>=0.24

possible to get them to work, but I'm skeptical if it's worth the effort to increase the complexity of the test infra to get there.

belm0 · 2024-06-09T02:50:26Z

@belm0 do you have strong opinions on keeping python3.7 support? It's getting problematic to support with lotsa libraries not supporting it.

I'm ok dropping 3.7 since it's past EOL. (For what it's worth, mainstream packages now think it's ok to drop versions before EOL-- numpy and others dropping 3.8 already.)

Testing trio versions < 0.22 is also getting messy. If we explicitly want to check for strict_exception_group compatibility the parameter was only introduced in 0.22 I would also love to use trio.testing.RaisesGroup, but at that point the test deps hit trio>=0.24

My org uses trio < 0.22 and would like to continue using the latest trio-websocket (otherwise I'm no longer using the package I maintain). My view is that trio's handling of the strict exception group transition is problematic (especially making strict_exception_group a runtime option that can have effect globally on transitive dependencies), so we're just avoiding it.

jakkdl · 2024-06-12T11:53:44Z

did some messing around:

Doing shield = True in close_connection is probably a bad idea when the disconnect_timeout defaults to a full minute. It might be a more proper thing to do, but that'd be a change of behavior that this PR does not care to mess with.
I looked at KeyboardInterrupt, and if a user were to cause one while _reader_task happens to be the current task, I think it will cause a TrioWebsocketInternalError. Would need to do signal handling in order to test it, or monkeypatch _reader_task or one of the handlers it uses to trigger this.

tests/test_connection.py

Makefile

belm0 · 2024-06-12T16:26:39Z

tests/test_connection.py

+    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))


Is this a mistake? I don't know why the lambda return expression includes a type annotation ([str]).

the annotation is for trio.open_memory_channel, specifying the type of the objects being sent over the channels. It's not normally possible to specify the type on functions, so Trio says it's a callable class during type checking to allow for that usage: https://github.com/python-trio/trio/blob/4dcdc6dbe899c9ee295fa9c8e6a077100ae19f16/src/trio/_channel.py#L91

Without the annotation, queued_streams would need the same long & verbose type annotation as accepted_streams.

belm0 · 2024-06-12T16:28:53Z

tests/test_connection.py

    ''' 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. '''
    async def handler(request):
        raise ValueError()

-    with pytest.raises(ValueError):
+    with pytest.raises((BaseExceptionGroup, ValueError)) as exc:  # pylint: disable=possibly-used-before-assignment


for cases like this, I would rather see an explicit assert exc in the code over a pylint annotation

it's BaseExceptionGroup that pylint fails to realize will always be available. But can switch to an assert

...except mypy correctly picks up that it will always be truthy, and raises a warning if doing assert BaseExceptionGroup >.>

tests/test_connection.py

belm0 · 2024-06-12T16:33:37Z

trio_websocket/_impl.py

+class TrioWebsocketInternalError(Exception):
+    ...
+
+


needs docs

If multiple non-Cancelled-exceptions are encountered, then it will raise TrioWebSocketInternalError with the exceptiongroup as its cause. This should only be possible if the background task and the user context both raise exceptions. This would previously raise a MultiError with both Exceptions.

other alternatives could include throwing out the exception from the background task, raising an ExceptionGroup with both errors, or trying to do something fancy with cause or context.

It seems odd to raise an internal error where a user-context exception is implicated. Perhaps we can discard the internal error with a warning message, and propagate the user exception.

It seems odd to raise an internal error where a user-context exception is implicated.

The "internalerror" bit would be that we're encountering a state that the internal logic assumed to be impossible - similar to TrioInternalError. I should've added a message to the exception to make that clearer.

Perhaps we can discard the internal error with a warning message, and propagate the user exception.

I quite like that solution. Only possible problem would be if this makes exception handling harder, but it 1. shouldn't happen, 2. they can still handle the user exception. We can also log detailed info on the exception we're suppressing, so the warning can be relatively brief.

trio_websocket/_impl.py

setup.py

belm0 · 2024-06-12T16:48:35Z

trio_websocket/_impl.py

WebSocketServer.run and WebSocketServer._handle_connection are the other two places that opens a nursery. I've opted not to change these, since I don't think user code should expect any special exceptions from it, and it seems less obscure that it might contain an internal nursery.

In principle I'd like to state that the API doesn't raise exception groups, and that would include the server API. For the user's handler, perhaps add UserRequestHandlerException?

I think that for users that are embracing strict_exception_groups=True and are used to handling exceptiongroups everywhere we shouldn't purposely obscure the groups and make errors harder for them to handle (and Trio's stance is that this is the default and should be embraced). I haven't delved too too deeply into them to see what it would imply to try and avoid exceptiongroups, but I think it might necessitate a change in API where we might end up changing what exceptions are being raised - even for users sticking with strict_exception_groups=False.

…t 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

…m 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

jakkdl · 2024-06-17T10:35:43Z

I rewrote the history with some squashing rebases, so even without merging #189 one can now selectively review this PR by only viewing 7eb779b in the "Files Changed" tab.

jakkdl · 2024-06-18T10:07:36Z

I looked at KeyboardInterrupt, and if a user were to cause one while _reader_task happens to be the current task, I think it will cause a TrioWebsocketInternalError. Would need to do signal handling in order to test it, or monkeypatch _reader_task or one of the handlers it uses to trigger this.

Okay I think I'm finally starting to understand KeyboardInterrupt. If either _reader_task or the user code raises KeyboardInterrupt, causing the other to get Cancelled, then we simply throw away the Cancelled and raise the original KeyboardInterrupt. But the more complex problem is if the user code raises some exception, and before we finish unwinding _reader_task and/or during close_connection we get a KeyboardInterrupt. At this point we definitely shouldn't suppress KI and just raise a warning, in favor of raising the users code. This is a prime example of when it would be so much easier to always raise a group, but I think what we ultimately have to do is raise ki_exception from user_exception.

Split out from #188 with a `--patch` checkout. This does destroy the commit history, so I might need to rewrite the history in #188 to avoid conflicts. * 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

…oups_support

…le cases

jakkdl · 2024-06-19T14:30:28Z

Big rewrite, fairly happy with the state of things - except:

the multiple added tests are very similar and could probably have common code moved into helpers/be parametrized/etc.
I had trouble engineering DisconnectionTimeouts, so there's no tests for exception chaining into that. I could of course monkeypatch that to happen though.

debugging old_deps is nooooo fun when the only exception being surfaced to me is errors inside TracebackException. 🙃

belm0 reviewed Jun 6, 2024

View reviewed changes

jakkdl marked this pull request as ready for review June 10, 2024 13:43

jakkdl mentioned this pull request Jun 12, 2024

Fixes for 3.13 python-trio/trio#3005

Merged

belm0 reviewed Jun 12, 2024

View reviewed changes

jakkdl mentioned this pull request Jun 17, 2024

drop 3.7, add mypy test, bump reqs, etc #189

Merged

jakkdl force-pushed the strict_exception_groups_support branch from 42f718f to 7eb779b Compare June 17, 2024 10:23

jakkdl added 4 commits June 19, 2024 11:43

Merge remote-tracking branch 'origin/master' into strict_exception_gr…

c3f7dc2

…oups_support

revamp exception handling, add multiple tests that cover most plausib…

8fd6db1

…le cases

fix pylint warnings

69853c3

fix old_deps

f22fa75

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support strict_exception_groups=True #188

Support strict_exception_groups=True #188

jakkdl commented Jun 6, 2024 •

edited

Loading

belm0 Jun 6, 2024 •

edited

Loading

jakkdl Jun 7, 2024

jakkdl commented Jun 7, 2024 •

edited

Loading

belm0 commented Jun 9, 2024

jakkdl commented Jun 12, 2024

belm0 Jun 12, 2024

jakkdl Jun 13, 2024

belm0 Jun 12, 2024

jakkdl Jun 13, 2024

jakkdl Jun 17, 2024

belm0 Jun 12, 2024

jakkdl Jun 13, 2024

belm0 Jun 12, 2024

jakkdl Jun 13, 2024

jakkdl commented Jun 17, 2024

jakkdl commented Jun 18, 2024 •

edited

Loading

jakkdl commented Jun 19, 2024

Support strict_exception_groups=True #188

Are you sure you want to change the base?

Support strict_exception_groups=True #188

Conversation

jakkdl commented Jun 6, 2024 • edited Loading

split out into #189

belm0 Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Jun 7, 2024 • edited Loading

belm0 commented Jun 9, 2024

jakkdl commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Jun 17, 2024

jakkdl commented Jun 18, 2024 • edited Loading

jakkdl commented Jun 19, 2024

jakkdl commented Jun 6, 2024 •

edited

Loading

belm0 Jun 6, 2024 •

edited

Loading

jakkdl commented Jun 7, 2024 •

edited

Loading

jakkdl commented Jun 18, 2024 •

edited

Loading