Cancelled __aexit__ handlers eat exceptions

This is what happens when you don't wrap your async context's exit handler in a shielded cancel scope:

… trick question. Nothing happens – the error gets dropped on the floor. Since this is a fairly common mistake (at least IME) I wonder whether we can do something about it. If not (which is what I suspect) we need to document this pitfall more loudly.

import trio

class bar:
    async def __aenter__(self):
        return self
    async def __aexit__(self, *tb):
        await trio.sleep(0)

async def foo():
    with trio.open_cancel_scope() as s:
        async with bar():
            s.cancel()
            raise RuntimeError("Duh")

trio.run(foo)

Sorry this has been biting you! I'd like to understand better what exactly's happening. I guess it must be something like:

Some code raises an exception
While that's propagating, the surrounding scope gets cancelled
While unwinding for the original exception, Python calls __aexit__, which calls some Trio primitive, which raises a Cancelled exception (probably this exception has the original exception stashed as its __context__, following Python's normal exception chaining rules)
Then the Cancelled exception gets swallowed by the original cancel scope, so the original exception is lost entirely

Does that sound right?

It would be helpful to hear more concrete details about how you managed to trigger this combination of circumstances -- maybe it would give clues about how it could be handled better.

#285 and #416 touch on related/overlapping issues.

I should also say I'm very wary about making a blanket recommendation that people use shielding in any __aexit__/finally blocks... it might be the best solution for now in your case, I don't know, but in the general case shielding is dangerous as well (what if the blocking code in the __aexit__ block gets stuck?), so hopefully there's a more specific solution we can come up with.

Ok I just got bit bad by something similar but incorporating async gens..
Makes sense but still was a nightmare to debug..

Check it:

import trio

at_exit_entered = False

class bar:
    async def __aenter__(self):
        return self
    async def __aexit__(self, *tb):
        print(tb)
        global at_exit_entered
        at_exit_entered = True
        await trio.sleep(0)


async def nums(seed):
    async with bar():
        for i in range(seed):
            await trio.sleep(0)
            yield i


async def iterate_nums():
    with trio.open_cancel_scope() as cs:
        async for i in nums(10):
            print(i)
            # cs.shield = True
            await trio.sleep(0.1)
            # cs.shield = False


async def foo():
    async with trio.open_nursery() as n:
        n.start_soon(iterate_nums)
        await trio.sleep(0.8)
        n.cancel_scope.cancel()


trio.run(foo)
# fails since ``bar.__aexit__()`` is never triggered
assert at_exit_entered

Un-commenting the cs.shield = lines of course makes it work.

One more super subtle cancellation fix

@tgoodlet ah, yeah, what you're hitting is a different frustrating problem. The problem is that neither you nor Python are taking responsibility for making sure your async generator object gets cleaned up properly. There's more discussion of this in PEP 533 (which is stalled, unfortunately). For now the only real workaround is to replace your async for with a clunky async with+async for combination:

from async_generator import aclosing

async def iterate_nums():
    ...
    async with aclosing(nums(10)) as ait:
        async for i in ait:
            ...

See also:

Some docs
Implement a strategy for handling (async) generator cleanup #265 discusses whether there's anything trio can do to mitigate this issue (though unfortunately we can't really fix it properly – in particular nurseries inside async generators absolutely require something like the aclosing hack).

BTW @smurfix, I'd still be interested in seeing how you managed to trigger the original issue in real code, in case you still remember. You say it's a common mistake, and I don't doubt you, but I also can't quite visualize how you're triggering it :-).

@njsmith

@njsmith ahah! ok I'll try this aclosing() out. Thx!

Also, fwiw, I have the same issue as @smurfix and have to specifically shield in order to get reliable cancellation working for actor nurseries.

One more super subtle cancellation fix

@oremanj

#746 was a duplicate of this.

@oremanj, can you give an example of what you were doing when you ran into this? I'm still having trouble getting an intuition for when it happens in practice :-)

There is a plausible justification for the current behavior: the task was going along, executing some arbitrary python code, which got cancelled. And in this case the arbitrary python code happened to be an exception handler. But of course what makes this counterintuitive is that we often think of finally or __aexit__ blocks as things that exceptions pass through, rather than places where they're caught, some logic executes, and then they're rethrown. I'm not really sure how to reconcile these different intuitions.

Over there @smurfix suggested that when an exception propagates into an __aexit__ method, and it raises a new exception, then maybe the two exceptions should be combined into a MultiError, instead of having the new exception replace the old exception. It's an interesting suggestion! Python's standard behavior here is sort of the only thing you can do in classic Python where MultiError doesn't exist, but now that we have MultiError maybe it should be revisited! There is one major obstacle that jumps to mind though, which is that one of the pillars of the MultiError v2 design (#611) is that each location either always wraps exceptions in MultiError, or never wraps exceptions in MultiError, and it would probably be surprising and annoying to have all exceptions that pass through channel.__aexit__ get wrapped into MultiError?

Though tbh we have this problem for nurseries too, and I'm not sure yet how that will all play out. Probably we should give MultiError v2 some time to shake out before we make decisions about __aexit__.

I'm not too fond of the "always-MultiError-or-never" idea. It's trivial to teach MultiError.split to work on non-MultiError exceptions – just package it into a MultiError and then return (None,exc)or (exc, None. There's a bit of boilerplate involved in that you need

except BaseException as exc:
    err, rest = MultiError.split(exc, RuntimeError)
    if err is None: raise
    … don't forget to think about what happens when `rest` is not `None`

instead of a plain

except RuntimeError as exc:

but that can't really be avoided in any case.

Yeah, it's trivial to teach MultiError.split and MultiError.catch to work on non-MultiError exceptions – in fact they already do, in the prototype – but it's less trivial to find all the places where except won't work, and has to be replaced by something like MultiError.catch.

Well, that gets more trivial if everything raises a MultiError, so you can't get away with handling the "old" exceptions.
It'll be interesting to see what kind of patterns emerge from all of this so that eventually the Python core can be extended appropriately. Hopefully.

I don't think this can work? Consider two cases:

Yeah I was thinking about this a bit today and came up with something similar that would work, but needing to see the future to know whether the exception would be reraised or not doesn't work.

(My idea was just to pretend that exact exception is a Cancelled for CancelScopes, i.e. store its id in a set somewhere... I guess just use a WeakSet.)

Maybe we should just admit the problem is intractable:

with trio.CancelScope() as cs:
  try:
    raise ValueError("...")
  except:
    cs.cancel()
    await trio.sleep(0)
    time.sleep(1)  # or anything that uses the result of an async thing, just to prevent us from continuing execution after being cancelled
    if random.choice([True, False]):
      raise

I would be alright with documenting that cancel points can act as a raise Cancelled statement (which would have the same behavior), or making changes and documenting as a raise statement, or alternatively warning about the potentially discarded exception.

@A5rocks

@A5rocks if it was possible to override nurseries or cancel scopes in some lowlevel. way, then it would be easier to prototype some approaches for coping with this imho.

As i think I've mentioned in trio matrix/gitter a couple times now, we've opted to (at least for the simplest case) unmask the Cancelled if a user writes app code which can result in remote error suppression, which in tractor's "SC enforcing supervision protocol" is somewhat of a larger issue since it breaks distributed-cross-process task-tree cancellation and error relay semantics..

The current soln is here for anyone interested:

first-draft impl of unwrapper: https://github.com/goodboy/tractor/blob/86346c27e874200a530d4151fbcfc535dc774d20/tractor/trionics/_taskc.py
usage by ._rpc sub-sys in our runtime: goodboy/tractor@4bc443c
test suite demonstrating how it works: goodboy/tractor@bad4273

I've been thinking a bit more about my second proposal:

making changes and documenting as a raise statement

And... well, here's what Trio would look like: main...A5rocks:trio:the-world-if.

Good news: it's really easy to fix any errors, simply traverse the call stack of an error until you get to a cancel point in an except:/finally:. Bad news: there's errors. Also the tracebacks include some internal Trio code...

I would be very interested in whether 3rd party code works with that branch, e.g. pip install git+https://github.com/A5rocks/trio@the-world-if.

I think deprecation is a bit tricky, but we can make up 2 new sync contextmanagers and it becomes trivial: (names tbd)

trio.cancel_should_suppress
trio.cancel_should_reraise (which we can eventually deprecate, but in the far far future)

Then we can warn if a cancellation gets caught by a cancel scope and a) hasn't gone through those two functions and b) will be an exception. I'm not too enthusiastic about such a large deprecation if there aren't too many benefits though.

I'm not sure what the benefit to introducing those contextmanagers is? Either of the behaviors are kinda bad even if you opt into one of them.
I think that it's simply ~always bad to have unshielded awaits (i.e. ASYNC120), so we could just start raising warnings and maybe eventually (or with a cm/strict parameter) raise a SuppressedExceptionError (either when catching the Cancelled, or instead of raising the Cancelled)

The problem with shielded awaits is liveliness concerns, I think (though I haven't really thought too much about it). Additionally, sometimes you do genuinely want to suppress/reraise on cancellation which I think are a bit convoluted! But the main benefit would be for this deprecation specifically, since we need some quick way for someone to write "actually I did mean to suppress the exception" that is future-compatible.

I was playing a bit more with that branch and I think the behavior in this case is worse:

try:
    1 / 0
except:
    with trio.CancelScope(shield=True) as cs:
        cs.cancel()
        await trio.sleep(0)

I'm not sure there's a set of rules which would exclude this case though. Is that enough to sink reraising as a default?

I've been experimenting with a similar context manager, "what if cancellation raised a group of [current_error, cancel]":

@contextmanager
def what_if_cancelling_raise_a_group():
    exceptions = [original_exc] if original_exc := sys.exc_info()[1] else []
    try:
        yield
    except BaseException as new_exc:
        exceptions.append(new_exc)
    if exceptions:
        raise BaseExceptionGroup("...", exceptions) from None

this seems to work pretty well in most cases - it's at least explicit - but there are a minority of cases where adding it breaks a lot of different things, and I'm not yet sure whether that's a fundamental issue or just a painful migration...

I'm not sure there's a set of rules which would exclude this case though. Is that enough to sink reraising as a default?

outermost cancel scope in active exception reraises the active exception instead of a Cancelled if would reraise the Cancelled
- not sure how exception groups work with this. I suppose we can just add the active exception to the exception group?
- or maybe anything that would create an exception group would replace the Cancelled with the active exception (only nurseries do this). I think this possibility is bad though.
otherwise, if there's no cancel scopes with the active exception, then the wait_until_rescheduled raises the active exception

Would be a set of rules that work. I'm not sure they're the right rules though!

there are a minority of cases where adding it breaks a lot of different things, and I'm not yet sure whether that's a fundamental issue or just a painful migration...

I think the issue here is that either we are providing an incomplete solution ("anyone with a linter will know to add this") or we do this for every cancellation. The latter is troubling because it becomes unknown whether any given checkpoint will be an exception group, and due to how exception groups are that's not good. While your own code can use except*: everywhere, for some reason CPython treats exception groups as second class citizens -- I've been told I need to document every function that can raise exception groups! and KeyboardInterrupt/SystemExit in an exception group don't set the exit code!

The problem with shielded awaits is liveliness concerns

ASYNC120 requires checkpoints to be shielded with a scope that also has a timeout. Though that still requires an arbitrary timeout and a delay in cancelling.

Additionally, sometimes you do genuinely want to suppress/reraise on cancellation which I think are a bit convoluted! But the main benefit would be for this deprecation specifically, since we need some quick way for someone to write "actually I did mean to suppress the exception" that is future-compatible.

aah, yeah that makes sense. 👍

ASYNC120 requires checkpoints to be shielded with a scope that also has a timeout. Though that still requires an arbitrary timeout and a delay in cancelling.

iirc we still recommend a timeout in the error message, but the linter will be satisfied without one too. (which contradicts the docs; we should probably update them...)

Hm, thinking through more things and just realized that in general try / except trio.Cancelled as a pattern would become incorrect if we reraise on cancellation. We can replace these cases with a cancel_should_suppress: (and we could set up a lint rule/autofix for this, I suppose? Or make cancellation exceptions not public s.t. the only way to catch them is to catch BaseException or use cancel_should_suppress + catch trio.Cancelled?)

try:
  with trio.cancel_should_suppress():
    ...
except trio.Cancelled:
  ...

But that doesn't seem great. Also things would become hard for anyio, unless they make all cancels suppress exceptions! Too bad there's no obviously right way to solve this. This is a point in favor of:

raising an exception group for cancellation, or
having a subtype of both Cancelled and the user exception

I still think both are too cursed though and in general unnecessary if we're willing to make a breaking change.

reraise-on-cancellation seems to me like it would be pretty confusing in cases where the except clause does not include a raise - to raise a new exception is of course nbd in Python, but to re-raise the existing exception without the usual syntactic indication feels pretty weird.

having a subtype also feels pretty cursed, given the nonsense that user-defined types can pull with __subclasshook__ etc.

So my favorite proposal so far is to raise a group-containing-cancelled, and include sys.exception() in that group unless we're inside with trio.cancel_should_suppress(): (or whatever we call that)

(I thought I was bitten by this, but actually I wasn't. Anyways, that prompted me to post my notes even though there's parts I'm not sure about yet)

I agree now that reraising is not an ideal solution. In an ideal world without backcompat worries, raising an exception group would work. However, we can still do a compromise.

trio.Cancelled becomes a base class
CancelledGroup inherits trio.Cancelled and BaseExceptionGroup
CancelledLeaf inherits trio.Cancelled (or we just use trio.Cancelled for leaf cancellations)
if there's an active exception then we raise CancelledGroup(..., [active exception, CancelledLeaf]) otherwise CancelledLeaf (or CancelledGroup(..., [CancelledLeaf]), more likely)
nurseries unwrap CancelledGroup maybe (see later)

There's a few surprises here. The one I was most surprised by was that except* trio.Cancelled will actually capture the CancelledGroup and so suppress everything.

So there's a few remaining questions with this approach:

should we expose the concept of CancelledLeaf for those except*s?
- if so, we cannot make Cancelled final for type checkers
- probably implies a CancelledBaseGroup as well as a CancelledGroup unless we say that "if you strip a CancelledLeaf from a CancelledGroup it becomes a regular exception group"
obviously the naming sucks...
should we still make it impossible to inherit Cancelled? (I think so)
should we keep the CancelledGroup around even after we get an actual exception group? it's a weird implementation detail that only exists for backcompat reasons.
how should we do the deprecation?
- this has been the main stopper to me just making a draft because I can't think of anything reasonable.

Here's the file I played around with to get more familiar with behavior. Since this is basically just a thought dump I haven't cleaned it up:

program

# try to wrap my head around having cancellation be an exception group
class Cancelled(BaseException):
    pass

class CancelledGroup(Cancelled, BaseExceptionGroup):
    # TODO: should this:
    #   - have a base exception/exception split?
    #      - only if it's possible to have one w/o Cancelled
    #   - be replaced with BaseExceptionGroup in nurseries?
    def derive(self, excs):
        print("derive", excs)
        return CancelledGroup(self.message, excs)

# class LeafCancelled(Cancelled):
#     # I wouldn't have thought this necessary, but it is?
#     # maybe just accept that `except* Cancelled` swallows the eg.
#     pass

# classic handling:
try:
    raise Cancelled
except Cancelled:
    print("cancellation!")

# new backwards compatible handling
try:
    raise CancelledGroup("cancellation", [Cancelled(), ValueError("!!")])
except Cancelled:
    print("cancellation!")

# new future-compatible handling
# note: this is surprising without a LeafCancelled()
try:
    raise CancelledGroup("cancellation", [Cancelled(), ValueError("!!")])
# except* ValueError:
#     raise
except* Cancelled:
    pass

# breaking change at some point

# async with trio.open_nursery() as nursery:
#     try:
#         5 / 0
#     except:
#         nursery.cancel_scope.cancel()
#         await trio.sleep(0)

# ^ should raise an exception group containing a zero div error

The one I was most surprised by was that except* trio.Cancelled will actually capture the CancelledGroup and so suppress everything.

This feels like a dealbreaker to me. I guess we could do a CancelGroup which does not inherit from Cancelled; that would allow users to catch it with plain starless except ...: if they wanted to - which improves on the pain point of raising BaseExceptionGroup("", [Cancelled, <sys.exception() if not None>]).

It does seem that raising a group is the way to go, and then we can presumably provide a context manager to discard the current exception on cancellation, for example (and a lint rule to make sure it's used if there's no raise, etc); maybe there would also be other semantics people would want.

I think the next step is probably to prototype this in a branch, and then we can see what using it in a real codebase feels like. Basically try out the whole migration to see if we should force everyone to do it - that's going to be a lot of work, but I don't see any better option!

should we expose the concept of CancelledLeaf for those except*s?

if so, we cannot make Cancelled final for type checkers

probably implies a CancelledBaseGroup as well as a CancelledGroup unless we say that "if you strip a CancelledLeaf from a CancelledGroup it becomes a regular exception group"

In my concept, the leaf type is our current Cancelled, and CancelGroup.derive() returns a vanilla (Base)ExceptionGroup if there are no remaining Cancelled exceptions.

how should we do the deprecation?

this has been the main stopper to me just making a draft because I can't think of anything reasonable.

Let's find out if the overall approach is viable first.

If so, I think we could do something similar to the strict-exception-groups transition (briefly opt-in, then opt-out, then local-opt-out, then done; and controlled by trio.run or nurseries or cancel scopes). This doesn't exactly spark joy, but it's at least something of a transition pathway which doesn't demand a big-bang migration.

We could also provide a temporary "switch passing new-style cancellation back to old-style" context manager, which would let you upgrade a callee before a caller. Otherwise we'd have to sweep bottom-up through the ecosystem, which is always very painful.

I guess we could do a CancelGroup which does not inherit from Cancelled; that would allow users to catch it with plain starless except ...: if they wanted to

I think that would be no different than always raising exception groups... The main reason why to inherit from Cancelled is so that code like:

try:
  await do_things()
except trio.Cancelled:
  undo_things()
  raise

keeps working. I suppose if we have a branch, both approaches are similar enough (just creating a special exception group and using it vs using normal exception groups) that it's easy to compare though.

"existing except Cancelled: keeps working" is definitely good; but "except* Cancelled: masks everything" means that we're back where we started in any code which is using that pattern. It's still partial progress, and maybe the best option for a while - we should prototype and see - but I don't think we can live with that long-term.

Migrating to except (Cancelled, CancelGroup): doesn't seem too onerous though, so I'm inclined to require that.

Navigation Menu

Repository navigation

Cancelled aexit handlers eat exceptions #455

17 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Cancelled __aexit__ handlers eat exceptions #455

Description

Activity

njsmith commented on Feb 23, 2018

njsmith commented on Feb 23, 2018

goodboy commented on Jul 13, 2018

njsmith commented on Jul 13, 2018

njsmith commented on Jul 13, 2018

goodboy commented on Jul 13, 2018

njsmith commented on Oct 23, 2018

m-u-xyz commented on Oct 23, 2018

njsmith commented on Oct 23, 2018

m-u-xyz commented on Oct 23, 2018

17 remaining items

A5rocks commented on Jun 18, 2025

goodboy commented on Jun 19, 2025

A5rocks commented on Aug 18, 2025

jakkdl commented on Aug 18, 2025

A5rocks commented on Aug 18, 2025

Zac-HD commented on Aug 18, 2025

A5rocks commented on Aug 18, 2025

jakkdl commented on Aug 19, 2025

Zac-HD commented on Aug 19, 2025

A5rocks commented on Sep 4, 2025

Zac-HD commented on Sep 4, 2025

A5rocks commented on Sep 18, 2025

Zac-HD commented on Sep 18, 2025

A5rocks commented on Sep 18, 2025

Zac-HD commented on Sep 18, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Cancelled aexit handlers eat exceptions #455