[Feature Request] temporalio.CancelledError should inherit from BaseException

Open
#1,292 8 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
48/100
Issue type
Feature
Clarity
Mostly clear
Activity status
Quiet
Tech stack
python
Domain
backend

Research direction

Start in temporalio/exceptions.py around the CancelledError definition at line 145, then review how cancellation is raised and handled across the SDK. Confirm the impact on existing Exception handlers and define the required compatibility behavior before changing the inheritance; done means cancellation is not inadvertently caught by broad Exception handlers and the relevant behavior is covered by tests.

Written by the indexing model from the issue text.

Description

enhancement
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.

Dominant language
Python
Stars
1.2k
Forks
241
Avg merge
3d 2h
Merged PRs (30d)
49

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from temporalio/sdk-python

All issues in temporalio/sdk-python

Similar issues

More Python issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.