Support strict_exception_groups=True

[jakkdl]

Fixes #132 and #187

• 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.

[jakkdl]

@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]

@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]

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.

[jakkdl]

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]

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.

[jakkdl]

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

1. the multiple added tests are very similar and could probably have common code moved into helpers/be parametrized/etc.
2. 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. :upside_down_face:

[fossdd]

hmm, i still got with this patch, failures running the streamlink testsuite...:

=============================================== short test summary info ===============================================
FAILED tests/test_api_validate.py::TestParseQsdValidator::test_failure - assert "ValidationEr...t 'int' (123)" == "ValidationEr...decode' (123)"
FAILED tests/webbrowser/cdp/test_client.py::TestEvaluate::test_exception - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/cdp/test_client.py::TestEvaluate::test_error - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/cdp/test_client.py::TestNavigate::test_detach - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/cdp/test_client.py::TestNavigate::test_error - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/cdp/test_connection.py::TestCreateConnection::test_failure - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/cdp/test_connection.py::TestReaderError::test_invalid_json - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/cdp/test_connection.py::TestReaderError::test_unknown_session_id - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/cdp/test_connection.py::TestSend::test_timeout[Default timeout, response not in time] - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/cdp/test_connection.py::TestSend::test_timeout[Custom timeout, response not in time] - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/cdp/test_connection.py::TestSend::test_bad_command - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/cdp/test_connection.py::TestSend::test_result_exception - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/cdp/test_connection.py::TestHandleCmdResponse::test_response_error - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/cdp/test_connection.py::TestHandleCmdResponse::test_response_no_result - ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
FAILED tests/webbrowser/test_webbrowser.py::TestLaunch::test_terminate_on_nursery_baseexception - BaseExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
=============================== 15 failed, 6375 passed, 28 skipped, 1 warning in 14.93s ===============================

[bastimeyer]

Apologies for the off-topic comment, but since I'm the maintainer of Streamlink, I'll have to answer the previous comment.

failures running the streamlink testsuite

trio-websocket is not actually involved in any of the tests.

Streamlink's webbrowser tests fully mock/stub/monkeypatch trio_websocket.connect_websocket_url / WebSocketConnection, because the tests are only interested in message handling, not how the messages are sent or received.

• https://github.com/streamlink/streamlink/blob/6.10.0/tests/webbrowser/cdp/test_connection.py#L173
• https://github.com/streamlink/streamlink/blob/6.10.0/tests/webbrowser/cdp/conftest.py#L8-L18
• https://github.com/streamlink/streamlink/blob/6.10.0/tests/webbrowser/cdp/__init__.py#L10-L33

Your test failures are caused by something else. Considering that other tests are failing as well, you should probably have a look at how you package trio. The Streamlink tests have been running just fine since the breaking changes of trio 0.25 were resolved. See GH actions running everything in a clean venv using the latest dependency versions.

• https://github.com/streamlink/streamlink/commit/1a7295b110777473575f41b868492fd56628b7df
• https://github.com/streamlink/streamlink/commit/78d0b58d757ada9049667dee3a7369d4e6b6dc9d
• https://github.com/streamlink/streamlink/commit/d5aa26f6f5d8290fef55dd0e11670d9e9096973d

tests/webbrowser/test_webbrowser.py::TestLaunch::test_terminate_on_nursery_baseexception

This test is part of an outdated version (changed in d5aa26f / 6.7.3).

tests/test_api_validate.py::TestParseQsdValidator::test_failure

• https://github.com/streamlink/streamlink/commit/0466622dc0bd13db972f6a00d2e2bda31ad50229