Can we talk about "cancelled" instead of "cancelled_caught" and "raised Cancelled" and similar circumlocutions?

[njsmith]

In this comment, @mehaase wrote:

Most low-level operations in trio provide a guarantee: if they raise trio.Cancelled, this means that they had no effect

I didn't understand this passage at first, because of the phrase "if they raise trio.Cancelled", which makes it sound like it got cancelled for some internal reason. From the user's point of view, it would make more sense to say, "if a call to send_all() is cancelled..."

I'm actually not sure what he's quoting, but it sure sounds like something I'd write in the docs, especially back when I was still struggling to come to grips with fine distinctions like cancel_scope.cancel_called versus cancel_scope.cancelled_caught. And I'm pretty sure the reason I didn't say "if a call to send_all() is cancelled" is because technically you could call .cancel() on a scope surrounding a call to send_all but the call didn't raise Cancelled because it had already passed the point of no return. Which... is technically accurate, but as Mark points out, super confusing to ordinary humans. And probably saying "if a call to send_all() is cancelled" would be totally clear to actual readers; if the tiny edge case was pointed out they'd just shrug and think "well, I guess it wasn't cancelled then".

And btw the distinction between .cancel_called and .cancelled_caught is also super confusing to regular humans.

Maybe we should change the official way of talking about this to more like: cancel_scope.cancel() requests a cancellation, but code might or might not actually be cancelled (= raise Cancelled).

That would suggest renaming the CancelScope attributes to something like .cancel_requested and .cancel_succeeded, and also probably updating the docs to use this language more uniformly (though I haven't read through the docs to check).

[smurfix]

That would suggest renaming the CancelScope attributes to something like .cancel_requested and .cancel_succeeded,

FWIW, I strongly support this.

The result of cancelling something is either (a) the called code didn't do anything, raising a Cancelled exception, or (b) the called code did what it was supposed to do, returning its result normally. Of course there's also the possibility of (c) the called code got part way through and left whatever it tried to accomplish in an inconsistent state.

It's probably out of Trio's scope to signal that state to the caller; there should be an attribute "is this object still usable", and/or the object should raise an InconsistentStateError when it's used again. We might want to document that as best practice, and maybe add that exception to Trio as a sensible default for trio-using libraries to raise.

[njsmith]

(a) the called code didn't do anything, raising a Cancelled exception [...] Of course there's also the possibility of (c) the called code got part way through and left whatever it tried to accomplish in an inconsistent state.

Unfortunately I don't think the cancel scope machinery has any realistic way to tell the difference between these two cases. I guess we could try doing something wacky like counting how many checkpoints there were (cancelled on first checkpoint = didn't do anything?), but I think that'd be super fragile in practice.

Anyway, the whole partial cancellation topic has been coming up a lot, so I opened #889 to consolidate the threads...

---

Back on the topic of this issue: I should also link to #886, which contains an idea that involves getting rid of .cancelled_caught entirely.

[smurfix]

Unfortunately I don't think the cancel scope machinery has any realistic way to tell the difference between these two cases.

I don't want it to. Deciding whether a cancelled .send_all is deadly is an application issue. SSL should mark the stream as unuseable. Somewhat along these lines (consider this fictional stream which needs to send a message header, followed by a body):

class Sender:
    def __init__(self):
        _sending = False
        self._sendLock = trio.Lock()
    async def send_all(self, head,body):
        async with self._sendLock:
            if self._sending: raise InconsistentStateError
            await self.stream.send_all(head)
            self._sending = True
            await self.stream.send_all(body)
            self._sending = False

If the stream in question can send an abort transmission in progress packet, we could transparently use that instead of raising the error.

Yes, this scheme needs to be supported by everything that has a send_all that can be interrupted part-way through, but I don't think doing anything else would be realistic, particularly considering SSL with its interesting protocol requirements.

[oremanj]

I definitely support a rename here! I'd suggest cancel_requested and cancelled as the names of the attributes. If/when we add grace periods, we could use cancel_forced for the one that tells you whether the grace period expired.

[belm0]

comment today by a colleague:

I basically avoid using cancelled_caught because it’s difficult to understand whether it means timeout or not.

If the cancel scope happens to be within a loop, it ends up being more readable to avoid the attribute altogether with something like:

while True:
    with trio.move_on_after(10):
        await foo()
        continue
    # do something here relevant to cancel-caught case only