[Feature Request] temporalio.CancelledError should inherit from BaseException

[worace]

Cancelled Error Deriving from Exception makes it easy to inadvertently ignore Cancellation Attempts

Currently temporalio.CancelledError inherits from temporalio.FailureError which in turn inherits from Exception.

I would like to suggest that the semantics of this type of error would be more appropriate if it derived from BaseException, primarily because of expectations and patterns of how existing application code might be handling those various cases.

Basically it is very common in user/app code to have fairly broad exception cases like except Exception as e: etc. This is arguably not "best practice" (you should really catch more specific individual exception types), but it happens a lot.

My understanding is that Temporal Cancellation errors are implemented via a fairly low-level Python C-extension API, which allows them to forcibly preempt arbitrary user code. This means those cancellation errors can surface at arbitrary points in the call-graph, i.e. they pop up in random places in User code, not just in the "outer layer" of Temporal workflow code.

This creates a scenario where it's easy for error handling logic in random application code to unintentionally ignore Temporal Cancellation attempts, which leads to lots of confusion when workflows that should have been cancelled keep running.

I believe this type of semantics is why the core Python runtime typically uses BaseException for things like hard system interrupts, cancellation errors, OOMs, etc. And you can actually see that the stdlib changed the base class of asyncio.CancelledError
from Exception to BaseException, I believe for similar reasons.

I realize that while this change is simple to implement it would have meaningful consequences for existing user code, but I wanted to propose the idea and see if this is something that has come up in discussion before, or if the community has any other suggestions on how to handle temporal CancelledError when working with existing code that may be handling Exception broadly.

[worace]

Here's a tiny code snippet indicating the kind of thing we are encountering

# This function could be executed by some temporal workflow or activity
def some_user_code(name: str) -> str:
    try:
        # 1. Temporal Cancelled error fires somewhere within this call stack
        return reticulate_splines(name)
    except Exception as e:
        # 2. Our existing broad exception handler catches the temporalio.CancelledError.
        #    It shows up in our internal logging system but the temporal workflow does not actually exit.
        logger.exception("encountered error when reticulating")
        return "mmm"

The thing is that this scenario is only possible because of the low-level thread interrupt API that Temporal is using to invoke Cancelled Errors. So it's not typically the case that random user code needs to handle exceptions that originate from entirely outside of its call stack.

[tconley1428]

It's definitely a sensible idea. temporalio.CancelledError is very comparable to asyncio.CancelledError. If it didn't already exist I would probably agree with you. In the current state, it would be difficult to change given our backwards compatibility guarantees, but we'll leave the issue open to track the concern. You could also try the community slack to see if anyone has some advice there.

[cretz]

Note, you can set @activity.defn(no_thread_cancel_exception=True) if you don't want the current exception to be raised inside the thread (you can still get when canceled via various methods on the activity module). If needed, we could entertain an improvement idea to customize the exception we raise (in decorator and maybe even in worker options, though we're trying not to add too many of those for such minor details).

But from a Temporal POV, cancellation is a serializable Temporal failure exception in the failure exception hierarchy of traditional exceptions and is plenty reasonable to catch, even though it does have parallels with asyncio.CancelledError which itself made this incompatible change in 3.8 because CPython doesn't have the extreme reliability guarantees we do.

[worace]

@cretz RE if you don't want the current exception to be raised inside the thread (you can still get when canceled via various methods on the activity module). So is the idea there that we would proactively check on the cancellation status in our workflow code? I suppose the main downside there is that you'll have potentially a longer duration between when the cancellation is received and when we act on it, since you have to wait for your callstack to return back to some point in the top level of the workflow where you are doing that voluntary cancellation check? That certainly seems reasonable, we'll look into that.

RE Temporal failure exception in the failure exception hierarchy of traditional exceptions and is plenty reasonable to catch I'm sure it's certainly reasonable to catch in cases where you are trying to do so. My point was more that the way the existing hierarchy is structured is likely at odds with how many users' existing codebases are structured, especially if they're migrating existing code from another workflow scheduling system onto temporal. Here's the thread discussing the asyncio change from Python 3.8 -- if I'm understanding correctly it seems to be very much the same thing we are discussing here: https://bugs.python.org/issue32528

In any case, thank you for the tip about a possible workaround, we'll check that out

[cretz]

So is the idea there that we would proactively check on the cancellation status in our workflow code?

Not workflow code, this is activity code.

We admittedly do an abnormal thing in Python where we interrupt a thread with an exception (we have to use the C API to do it, normal Python has no such facility). But you can disable this abnormal thing and check for cancellation in your thread as you may with other Python libraries which is a bit more reactively if I understand correctly. For instance, we offer temporalio.activity.is_cancelled() and temporalio.activity.wait_for_cancelled_sync() (both of which are effectively backed by a threading.Event).

My point was more that the way the existing hierarchy is structured is likely at odds with how many users' existing codebases are structured

Agreed, not only the hierarchy is at odds, but interrupting a thread via a custom exception at all is at odds with how many user's codebases are structured. We would use Python cancellation/interruption for threads if such a thing were standardized (same as we do for asyncio). In the meantime, would definitely suggest turning it off for codebases not structured for this kind of exception injection approach (regardless if because of exception type or because it is occurring at all).

We can't alter things for backwards compatibility reasons, but if really needed, we can look into allowing you to customize the exception type we raise inside running threads.

[worace]

Got it, that makes sense. Thanks for that context. We will explore some of these options, it sounds like the opt-in cancellation checks will probably work for our use case. It may take me a bit of time but I can report back on how that works out if it would be helpful.

I'll leave it up to you whether you want to leave this issue open or close it. If nothing else it may provide useful documentation for anyone searching for similar issues in the future.

Your suggestion of making the exception types customizable would certainly work for our use case as well, and would have the benefit of not requiring any other changes to our activity code. But I realize it's kind of a complicated option to incorporate into the API for what may be a relatively niche problem.

[cretz]

👍 Will circle back with team on custom exception in this case and leave the issue open until direction determined.