Cancel Temporal timer when workflow.sleep() task is cancelled#1352
Open
carlosa54 wants to merge 1 commit intotemporalio:mainfrom
Open
Cancel Temporal timer when workflow.sleep() task is cancelled#1352carlosa54 wants to merge 1 commit intotemporalio:mainfrom
carlosa54 wants to merge 1 commit intotemporalio:mainfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed
When
workflow.sleep()is wrapped in anasyncio.Taskand that task is cancelled, two things now happen correctly:set_result()on an already-cancelled future, preventingasyncio.InvalidStateErrordone_callbackon the future propagates cancellation to the underlying Temporal timer viatimer_handle.cancel(), which sends acancel_timercommand to the serverPreviously,
workflow.sleep()discarded the_TimerHandlereturned by_timer_impl(), so there was no way to cancel the Temporal timer when the awaiting task was cancelled. This is in contrast toasyncio.sleep(), which goes through the event loop'scall_later()path where Python's asyncio internals callhandle.cancel()automaticallyA new test (
test_workflow_sleep_task_cancellation) mirrors the existingCancelSignalAndTimerFiredInSameTaskWorkflowtest but usesworkflow.sleep()instead ofasyncio.sleep().Why?
workflow.sleep()andasyncio.sleep()should behave the same way when cancelled.Without this fix:
asyncio.InvalidStateErroris logged when the timer fires after the task is already cancelled.TimerCanceledevent in history), leaking server-side resources.See Issues #782 and #1351 for the bug reports
Checklist
Closes [Bug] cancelled timer callback causes asyncio.exceptions.InvalidStateError #782
Closes [Bug] workflow.sleep() timer not canceled when wrapping asyncio.Task is canceled #1351
How was this tested:
test_workflow_sleep_task_cancellationwhich starts aworkflow.sleep(60 * 60)in a task, cancels it via signal, and asserts the workflow completes with"timer_cancelled"(noInvalidStateError)test_workflow_cancel_multi,test_workflow_cancel_signal_and_timer_fired_in_same_task,test_workflow_sleep,test_concurrent_sleeps_use_proper_optionsAlso tested locally:
Before

After
