Skip to content

Improve TimerTask#122

Open
andystaples wants to merge 9 commits intomicrosoft:mainfrom
andystaples:andystaples/add-task-cancellation
Open

Improve TimerTask#122
andystaples wants to merge 9 commits intomicrosoft:mainfrom
andystaples:andystaples/add-task-cancellation

Conversation

@andystaples
Copy link
Copy Markdown
Contributor

Adds the following to improve TimerTask:

  1. Adds support for task cancellation, enabling patterns like:
    def orchestrator(ctx: task.OrchestrationContext, _):
        approval: task.Task[bool] = ctx.wait_for_external_event('Approval')
        timeout = ctx.create_timer(timedelta(seconds=3))
        winner = yield task.when_any([approval, timeout])
        if winner == approval:
            # Explicitly cancel the timer so it does not linger
            timeout.cancel()
            return "approved"
        else:
            return "timed out"
  1. Adds support for LongTimers with configurable max_timer_interval, so that backends with limits on timer duration can be used

Resolves #95
Resolves #101

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the Durable Task Python SDK’s timer functionality by adding (1) task cancellation support for timers/external-event waits and (2) long-timer chunking to accommodate backends with maximum timer duration limits (resolving #95 and #101).

Changes:

  • Introduces CancellableTask/TaskCanceledError and wires cancellation into timer + external event tasks.
  • Adds LongTimerTask and updates orchestration execution to chunk long timers based on a maximum interval (default 3 days).
  • Expands orchestration executor tests to cover long-timer chunking, cancellation after when_any, and long retry timers.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File Description
durabletask/worker.py Implements long-timer chunk scheduling in the runtime context and executor; adds a maximum_timer_interval configuration knob.
durabletask/task.py Adds cancellable task primitives and long-timer task implementation.
tests/durabletask/test_orchestration_executor.py Adds unit tests for long-timer chunking/cancellation and retry behavior with long timers.

You can also share your feedback on Copilot code review. Take the survey.


def __init__(self) -> None:

class LongTimerTask(TimerTask):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than a dedicated LongTimer class - try to make the existing Timer support long timers also. Similar how to dotnet has a single CreateTimer. https://github.com/microsoft/durabletask-dotnet/blob/45ab40eeff0090f5124a98d0ca15148b144cf79c/src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs#L266

Copy link
Copy Markdown
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task cancellation support is a great addition — the CancellableTask pattern with cancel handlers for timers and external events is well-designed and the tests are thorough.

However, the long timer implementation introduces a separate LongTimerTask class that diverges from the .NET design. In the .NET SDK (TaskOrchestrationContextWrapper.cs#L266-L283), long timer chunking is handled entirely within the CreateTimer method using a simple loop — there is no separate timer type. The chunking is an implementation detail of the orchestration context, not a property of the task itself.

In Python we should not have two different classes for timers. Instead, the existing TimerTask should be updated to support long timers internally. The chunking logic should live in create_timer_internal and the timer-fired event handler, with TimerTask optionally holding the final_fire_at and maximum_timer_interval state when needed. This keeps the type hierarchy simple and aligns with the .NET design where the long timer behavior is transparent to consumers.

Note: The PR has a merge conflict (mergeable_state: dirty).

Disclaimer: This review was generated by GitHub Copilot on behalf of Bernd.

return self._final_fire_at


class WhenAnyTask(CompositeTask[Task]):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .NET SDK handles long timer chunking entirely within TaskOrchestrationContextWrapper.CreateTimer() (source) — it does not have a separate LongTimerTask type. The chunking is transparent; the same Task object is awaited multiple times in a loop.

Please merge LongTimerTask functionality into TimerTask directly. TimerTask can optionally hold _final_fire_at and _maximum_timer_interval (defaulting to None) and the complete() method can decide whether to return a next fire time or actually complete. This keeps a single timer type and aligns with the .NET approach where long timers are an implementation detail, not a type distinction.

class TimerTask(CancellableTask[None]):
    def __init__(self, final_fire_at: Optional[datetime] = None,
                 maximum_timer_interval: Optional[timedelta] = None):
        super().__init__()
        self._final_fire_at = final_fire_at
        self._maximum_timer_interval = maximum_timer_interval

    def set_retryable_parent(self, retryable_task: RetryableTask):
        self._retryable_parent = retryable_task

    def complete(self, current_utc_datetime: datetime) -> Optional[datetime]:
        if (self._final_fire_at is not None
                and self._maximum_timer_interval is not None
                and current_utc_datetime < self._final_fire_at):
            return self._get_next_fire_at(current_utc_datetime)
        super().complete(None)
        return None

    def _get_next_fire_at(self, current_utc_datetime: datetime) -> datetime:
        if current_utc_datetime + self._maximum_timer_interval < self._final_fire_at:
            return current_utc_datetime + self._maximum_timer_interval
        return self._final_fire_at

Disclaimer: This review was generated by GitHub Copilot on behalf of Bernd.

return
if not (isinstance(timer_task, task.TimerTask) or isinstance(timer_task, task.LongTimerTask)):
if not ctx._is_replaying:
self._logger.warning(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With LongTimerTask being a subclass of TimerTask, the isinstance(timer_task, task.LongTimerTask) check is redundant — isinstance(timer_task, task.TimerTask) already covers it. This further highlights that having two classes is unnecessary; a single isinstance(timer_task, task.TimerTask) check would suffice.

Disclaimer: This review was generated by GitHub Copilot on behalf of Bernd.

def set_retryable_parent(self, retryable_task: RetryableTask):
self._retryable_parent = retryable_task

def complete(self, _: datetime) -> None:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The complete() method now takes a datetime parameter and returns Optional[datetime], which is a different signature from the parent CompletableTask.complete(result: T). This works but is a bit confusing — the base complete expects a result value while TimerTask.complete takes a timestamp and has a different return type. Consider either renaming this (e.g., _handle_timer_fired) to make it clear it's internal timer plumbing, or aligning the method signatures more cleanly. The super().complete(None) call inside already shows the impedance mismatch.

Disclaimer: This review was generated by GitHub Copilot on behalf of Bernd.

self._parent.on_child_completed(self)


class CancellableTask(CompletableTask[T]):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CancellableTask design with pluggable _cancel_handler callbacks is clean. It correctly handles the idempotent case (returning False if already complete) and integrates well with the composite task pattern via on_child_completed. Good alignment with the .NET CancellationToken pattern adapted to Python's generator-based orchestrations. 👍

Disclaimer: This review was generated by GitHub Copilot on behalf of Bernd.

concurrency_options=concurrency_options)
concurrency_options=concurrency_options,
maximum_timer_interval=None # DTS allows timers of indefinite length
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the .NET SDK, MaximumTimerInterval defaults to 3 days in DurableTaskWorkerOptions and is not overridden for DTS — DTS workers still use the default 3-day maximum. Here you're passing None to disable chunking entirely for DTS. Can you confirm this is the intended behavior? If DTS truly supports indefinite timers, the None is fine, but it'd be good to document the rationale (e.g., "DTS natively supports long timers so chunking is unnecessary").

Disclaimer: This review was generated by GitHub Copilot on behalf of Bernd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for long timers Provide an API to cancel a Task

3 participants