Cancelled AsyncResources eat exceptions

[oremanj]

This is an especially surprising special case of #455.

This code:

with trio.move_on_after(1):
    async with any_asyncresource:
        try:
            await trio.sleep_forever()
        finally:
            raise ValueError

will swallow the ValueError, because it gets stuck as the __context__ of the Cancelled exception raised in any_asyncresource.aclose. And aclose here is basically required to raise Cancelled, because Trio rules require it to be a checkpoint.

This showed up in #1555, with trio.Process standing in for "any_asyncresource".

This kind of thing keeps showing up for me and it's extremely confusing to track down. I know the party line is "who knows if someone was going to handle that exception that got stuck as the Cancelled __context__, we shouldn't reraise it when we swallow the Cancelled". But I think sometimes raising an exception that the user thought was handled is a much better failure mode than sometimes silently swallowing an exception that no one tried to handle at all.

[smurfix]

the context of the Cancelled exception

Ah. So that's where these vanish to.

I agree, though I do wonder how many false positives a change like that would trigger.

[njsmith]

I really don't like the idea of an exception "teleporting" from the place where the resource is closed up to the place where the move_on_after is closed.

Instead, let's consider if there's some way to let the original exception keep propagating through the resource __aexit__, so it never gets replaced by Cancelled in the first place.

You could make an argument that exceptions coming into AsyncResource.__exit__ should always trigger an ungraceful shutdown. That would make this easy to solve:

class AsyncResource:
    async def __aexit__(self, etype, value, tb):
        if etype is not None:
            with trio.CancelScope(shield=True):
                await aclose_forcefully(self)
        else:
            await self.aclose()

I'm not quite sure I understand the full implications of this, though. It's definitely less surprising in the case where you have a ambient cancellation ongoing and a non-Cancelled exception propagating. But the more common case is where you have non-Cancelled exception and no ambient cancellation, and I'm not sure if this is what users want or expect there.

Another option would be to say that in AsyncResource.__aexit__, we want incoming exceptions to "overwrite" Cancelled, but otherwise keep things the same. Maybe

class AsyncResource:
    async def __aexit__(self, etype, value, tb):
        try:
            await self.aclose()
        except Cancelled:
            if etype is not None:
                return  # let original exception propagate instead
            raise

This doesn't handle MultiErrors correctly, though.

[oremanj]

I think cancelling aclose on any exception is a little aggressive, but I like your second proposal. I think it can be extended to support MultiErrors just fine -- if everything in the MultiError is a Cancelled, then treat it like a Cancelled, otherwise let it keep propagating because the original exception will be preserved as the MultiError's __context__ and the MultiError won't be completely eaten by the cancel scope.

Ideally this would be a context manager we expose publicly that people can put around their own async finally: blocks and such, too. Maybe something like

@contextmanager
def active_exception_propagates_on_cancel():  # better name suggestions welcome
    if sys.exc_info()[0] is None:
        # no active exception, so nothing to do
        yield
        return
    # we have an exception, so don't allow Cancelled to propagate since that
    # would hide the current exception
    def remove_cancels(exc):
        return None if isinstance(exc, trio.Cancelled) else exc
    with MultiError.catch(remove_cancels):
        yield

[smurfix]

the more common case is where you have non-Cancelled exception and no ambient cancellation

… except when the non-Cancelled exception triggers a cancellation which triggers another exception – which then gets swallowed up by said cancellation. That happens all-too-often in network clients (or servers for that matter) when handling teardown of the connection is … rather less well-tested than we'd like.

[graingert]

except when the non-Cancelled exception triggers a cancellation which triggers another exception

I'd really like to see a code example for that because I can't wrap my head around how it would look