-
Notifications
You must be signed in to change notification settings - Fork 140
Nexus task cancellation #1204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Nexus task cancellation #1204
Conversation
…_manager. Finish implementing nexus task cancellation
…hat weren't relevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking
| class _NexusTaskCancellation(nexusrpc.handler.OperationTaskCancellation): | ||
| def __init__(self): | ||
| self._thread_evt = threading.Event() | ||
| self._aysnc_evt = asyncio.Event() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self._aysnc_evt = asyncio.Event() | |
| self._async_evt = asyncio.Event() |
| async def wait_until_cancelled(self) -> None: | ||
| await self._aysnc_evt.wait() | ||
|
|
||
| def cancel(self, reason: Optional[str] = None) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably no need to return a never-read value from an effectively module-private method, but meh
| async def wait_until_cancelled(self) -> None: | ||
| await self._aysnc_evt.wait() | ||
|
|
||
| def cancel(self, reason: Optional[str] = None) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a possibility of reason being none? Even if there is (some newer proto value on the server that we don't have an entry for yet), is there any value in defaulting this parameter (or the one on the cancel call that calls this)?
| await self._aysnc_evt.wait() | ||
|
|
||
| def cancel(self, reason: Optional[str] = None) -> bool: | ||
| if self._thread_evt.is_set(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No real value in Python in double-checked in and out of lock, can just rely on the in-lock check (arguably lock not as valuable in Python as it is in other langs, could just change top of cancellation_reason to have an if self._thread_evt.is_set() instead of the lock, but it's fine)
What was changed
Why?
Nexus operation handlers should be able to handle the case where a call
start_operationor call tocancel_operationis interrupted. The most common occurrence of this is a timeout where the call fails to return in time. The new cancellation objects attached to the Nexus operation context allows both sync and async functions to exit early when appropriate.These changes rely on nexus-rpc/sdk-python#31 and is the Temporal specific implementation of nexus-rpc/sdk-python#26.
Checklist
test/nexus/test_workflow_caller_errors.pywere updated to cover: