MultiError.catch should take an exception type, like except: does

[njsmith]

In regular Python, a catch-all exception handler looks like:

try:
    ...
except Exception as exc:  # Don't catch KeyboardInterrupt, etc.
    logger.error(..., exc_info=exc)

In Trio we need to handle MultiErrors, which might contain a mix of exceptions we want to catch and ones we don't, etc. So you have to use MultiError.catch, and the above becomes:

def handler(exc):
    if isinstance(exc, Exception):
        logger.error(..., exc_info=exc)
        return None  # swallow this exception
    else:
        return exc  # let other exceptions propagate

with MultiError.catch(handler):
    ...

This is pointlessly cumbersome. We should make it look like this:

def handler(exc):
    logger.error(..., exc_info=exc)

with MultiError.catch(Exception, handler):  # Note the extra argument
    ...

[WindSoilder]

For the newer version of catch, if we have

with MultiError.catch(RuntimeError, handler):
   ...

Does it means that we want the MultiError filter out the RuntimeError (and Cancelled) ?

If so, what if we want to keep the behavior default? (just filter out the Cancelled exception)....

May be just pass None to catch? So the code would be something like:

with MultiError.catch(None, handler):
   ...

[njsmith]

@WindSoilder the idea is that

with MultiError.catch(RuntimeError, handler):
    ... body ...

is a substitute for

try:
    ... body ...
except RuntimeError as exc:
    handler(exc)

The work the same except the first one handles MultiErrors.

When you talk about filtering Cancelled, you're thinking of what cancel scope blocks do. MultiError itself doesn't know anything about cancellation. In fact, open_cancel_scope uses MultiError.catch to catch Cancelled exceptions. So that's a higher level than all this.

[WindSoilder]

@njsmith Thanks for your comment! It makes me more clear about this.

But there is something still confuse me, take the code as example:

with MultiError.catch(Cancelled, handler):
    ... body ...

According to your description, when handler is called, it's argument type(Exception) can be RuntimeError or MultiError.

I have take a look at _exc_filter in CancelScope, when MultiError.catch take an exception type, _exc_filter still need to check if the exception type is Cancelled, because it can be MultiError.

Maybe this question is too foolish, but it still confuse me...I still don't understand the value of add an exception type to catch :(
May be it's better to just let handler get Cancelled exception? Then Multierror can be handled in MultiErrorCatcher

[njsmith]

With the current MultiError design, the catch handler never gets passed MultiError objects. I'm not sure what you saw that makes you think otherwise. Currently, the handler gets called on all the basic exception objects, so e.g. in MultiError([Cancelled(), ValueError()]), the _exc_filter in CancelScope would get called twice: one with Cancelled and once with ValueError. So the isinstance check you're seeing is to ignore the ValueError. The original idea for this issue was to simply move this check into MultiError.catch, since otherwise every handler ends up reimplementing it.

With the "MultiError v2" proposal (#611), this changes slightly, and the exception type becomes more important. This proposal isn't implemented yet. But here, the idea is MultiError.catch(RuntimeError, handler) takes whatever exception comes out of the code block, and then splits it into two halves: the RuntimeError half and the non-RuntimeError half. This uses the split operation described in #611. And then it calls the handler on the RuntimeError half. This might be a simple RuntimeError, or it might be a MultiError... but if it's a MultiError, then it's one that only contains RuntimeErrors. So again, the handler doesn't have to check the exception type explicitly: it knows that if it's running at all, it's because there was at least one RuntimeError.

[belm0]

I haven't found catch() to be useful, and this improvement wouldn't help.

The problem with the catch API is that the handler can only see one exception at a time. So far I've needed to see all exceptions to decide the desired filtering.

Scenario: I want to defer any exceptions raised by my async API to Cancelled if they are raised simultaneously (e.g. if move_on_after expires I'm fine dropping any API exceptions). So if MultiError contains Cancelled, then filter my API's exceptions. It can't be done with catch().

[njsmith]

@belm0 Interesting... are you up for writing a longer description of what you're trying to do in the v2 thread, #611? That's where the main design discussions have been happening for the MultiError rewrite, and so far everything has been based around the simplifying assumption that the way you handle one exception type in a MultiError isn't affected by the presence or absence of other exception types...

[Zac-HD]

Closing this because MultiError.catch() is deprecated in favor of exceptiongroup.catch() (or except*, on Python 3.11).