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

New API for handling concurrent exceptions #1683

Closed
bob1de opened this issue Aug 13, 2020 · 9 comments
Closed

New API for handling concurrent exceptions #1683

bob1de opened this issue Aug 13, 2020 · 9 comments

Comments

@bob1de
Copy link

bob1de commented Aug 13, 2020

Hi,

I wasn't sure whether this should go into the exceptiongroup repo or not, so I'm opening the issue here because I implemented and use this with MultiError in current trio. However, it can be ported to ExceptionGroup easily.

I needed to catch concurrent exceptions at various places and found it quite unergonomic to use MultiError.catch the way handlers need to be implemented right now.

So I thought back and forth about it and came up with a context manager-based approach where the code inside the context is guarded by exception handling and handlers for individual exceptions can be registered using decorators. Both sync and async handlers are supported, and it also allows registering handlers for the else and finally branches so that one can avoid another try block + the extra indentation.

The code can be found here:
https://github.com/efficiosoft/trio_catcher

Docs and some thoughts are provided in README and docstrings.

I'd be very interested in what you think about such an approach. Imho, at least it's the cleanest-looking I've found so far. :-)

Best regards
Robert

@njsmith
Copy link
Member

njsmith commented Aug 14, 2020

It sounds pretty cool, similar to the idea in python-trio/exceptiongroup#5 (and linked issues). The current API is definitely pretty bad.

In particular, this comment has some details on exactly what semantics we've been thinking about, including all the annoying edge cases that can happen: python-trio/exceptiongroup#5 (comment)

Do you think you could write a short comparison of how your semantics compare to those?

@bob1de
Copy link
Author

bob1de commented Aug 14, 2020

I can try to write a comparison soon.

However, I think the semantics need some rethinking anyway. As it is right now, a handler can consume only one exception at a time and either replace that with another one or drop it. What isn't possible is consuming two semantically linked exceptions in one handler or, the other way around, replacing a single exception with many.

What do you think, would it make sense to allow a handler to subscribe to multiple exceptions in an AND manner, so that it is only executed when all exceptions are present and consumes them all with a single call? Likewise, it could return a tuple of exceptions for inclusion into the resulting MultiError. One could thus provide both a handler for e.g. ValueError+OSError, one for ValueError and one for OSError alone.

I wanted to swap the two central for loops in aexit anyway, so that the outer one iterates over handlers, which would make such a thing possible.

It gets tricky however when a handler wants e.g. TimeoutError + OSError and exceptions are checked in wrong order, so we would need to enforce that no subtype follows its supertype at handler registration time, but that should be no problem.

@bob1de
Copy link
Author

bob1de commented Aug 14, 2020

I just tried to implement selective consumption of multiple exceptions in the except-multiple branch. Seems to work so far. Docstrings weren't updated yet, but it should be understandable from the code for now.

@bob1de
Copy link
Author

bob1de commented Aug 14, 2020

So here is a little comparison to trio/exceptiongroup#5. Just what I found so far, so no guarantees that this is complete or 100% correct of course:

https://github.com/efficiosoft/trio_catcher#comparison-to-trioexceptiongroup5

EDIT: I also added sample.py which is just the example from docstring, but readily runnable.

@bob1de
Copy link
Author

bob1de commented Aug 15, 2020

I don't want to bother you too much, but there's another even simpler approach that came to my mind:

https://github.com/efficiosoft/trio_catcher#manual-programmatic-handling

Maybe this one is even better, at least its semantics are obvious just from looking at some code using it and it doesn't reinvent the wheel. It does require another try block around the nursery, but even then it still looks quite promising to me.

And of course the functions in trio_except.py would need to be staticmethods of MultiError/ExceptionGroup for this to feel ergonomic.

EDIT: Or maybe even real methods in the case of ExceptionGroup. Since they don't collapse and a nursery will then always raise ExceptionGroup even with a single exception, one could confidently except ExceptionGroup instead of BaseException and then call the methods on the caught exception directly.

@bob1de
Copy link
Author

bob1de commented Sep 5, 2020

Hi again,

Do you want me to create a PR to add the second, function-based approach to MultiError/ExceptionGroup? After having worked with it for some time, it feels pretty comfortable and so far I had no cases it couldn't handle.

Is there already a plan for the migration process to ExceptionGroup?

@oremanj
Copy link
Member

oremanj commented Sep 7, 2020

Thank you for posting these examples! They definitely provide some useful food for thought. It's especially useful to know that they've served you well in real programs.

I think we probably don't want to make many further changes to upstream MultiError at this point (since it's something of an evolutionary dead end given the desire to switch to ExceptionGroup) but ExceptionGroup will definitely want some sort of ergonomic exception-handling support IMO. You're welcome to post a PR, but given current limitations on reviewer bandwidth, it's unfortunately unlikely to be merged soon -- you're adding quite a lot of API surface area, which we generally try to be quite cautious and deliberate about due to backwards-compatibility concerns. Conveniently, though, the functionality you're adding doesn't really need to be in the main Trio package -- it seems like it would work just as well as a library. So if you want to get this into the hands of more people, it might be worth packaging and sticking on PyPI so those interested can use it and provide feedback?

@bob1de
Copy link
Author

bob1de commented Sep 7, 2020

I think we probably don't want to make many further changes to upstream MultiError at this point (since it's something of an evolutionary dead end given the desire to switch to ExceptionGroup)

Sounds reasonable. It would probably cause more confusion than good to add another API to MultiError in this stage. But then again, when is this switch planned for?

but ExceptionGroup will definitely want some sort of ergonomic exception-handling support IMO.

The try/except-based approach only makes real sense with ExceptionGroup anyway, but there it shines.

You're welcome to post a PR, but given current limitations on reviewer bandwidth, it's unfortunately unlikely to be merged soon -- you're adding quite a lot of API surface area, which we generally try to be quite cautious and deliberate about due to backwards-compatibility concerns.

I did so for ExceptionGroup (python-trio/exceptiongroup#22). Due to the way ExceptionGroup is designed (especially the additional list of sources), this required rethinking the API a bit, but I now added five methods, all with docstrings and at least started with some unittests. Full test coverage will be done of course in case the idea is accepted.

Fortunately we don't have to worry about backwards compatibility with ExceptionGroup yet, but nevertheless it doesn't break anything in a backwards-incompatible way.

Conveniently, though, the functionality you're adding doesn't really need to be in the main Trio package -- it seems like it would work just as well as a library. So if you want to get this into the hands of more people, it might be worth packaging and sticking on PyPI so those interested can use it and provide feedback?

It requires some monkey patching of MultiError and would be possible, but as you said, it's probably a bad idea to introduce something new for it now. ExceptionGroup fits this concept much better anyway.

@Zac-HD
Copy link
Member

Zac-HD commented Oct 30, 2022

Closing this now that Python 3.11 has been released with PEP 654 – Exception Groups and except*, and Trio has switched over.

@Zac-HD Zac-HD closed this as completed Oct 30, 2022
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

No branches or pull requests

4 participants