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

Hide MultiErrors #132

Closed
mehaase opened this issue Feb 28, 2020 · 0 comments · Fixed by #188
Closed

Hide MultiErrors #132

mehaase opened this issue Feb 28, 2020 · 0 comments · Fixed by #188

Comments

@mehaase
Copy link
Contributor

mehaase commented Feb 28, 2020

Several of the APIs in this library create a "hidden" nursery that runs the connection's background task. This has the surprising side effect that something that looks like single-task code can actually raise a MultiError. Nathaniel has a good thread describing this. I'll just steal a snippet to illustrate the problem here:

async with open_websocket("https://...") as ws:
    raise ValueError

but if open_websocket has a nursery hidden inside it, then you'll get a MultiError([ValueError]) instead of a ValueError, and that's going to surprise people, because it's mostly an implementation detail that open_websocket creates a nursery.

The proposed solution is to design the library to avoid raising exceptions on the background task. This would allow us to catch MultiError in the hidden nursery, unwrap the nested exception, and raise it to the caller. If the background task does raise an error, then we wrap* the entire MultiError inside of a new class TrioWebsocketInternalError. This wrapper class is a signal to the caller that the bug is in this library, not in their code.

*Nathaniel's post suggests that the internal error either is-a or has-a MultiError. We can also look at TrioInternalError for inspiration.

belm0 pushed a commit that referenced this issue Sep 21, 2024
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 `KeyboardInterrupt`s
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant