-
Notifications
You must be signed in to change notification settings - Fork 53
Add WaitForExternalEvent overload with timeout and cancellation token #555
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
Conversation
…ionToken Co-authored-by: YunchuWang <[email protected]>
Co-authored-by: YunchuWang <[email protected]>
Co-authored-by: YunchuWang <[email protected]>
8d456b7 to
da251bc
Compare
|
change the PR description to the latest PR template in https://github.com/microsoft/durabletask-dotnet/blob/main/.github/PULL_REQUEST_TEMPLATE.md |
|
@copilot add test for External cancellation token fires |
|
@copilot update pr description to follow .github/PULL_REQUEST_TEMPLATE.md |
Co-authored-by: YunchuWang <[email protected]>
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.
Pull request overview
This PR adds a missing WaitForExternalEvent<T> overload that accepts both a timeout and cancellation token, enabling users to implement race scenarios where multiple events compete with individual timeouts and the winner can cancel the losers.
Key changes:
- Added new overload
WaitForExternalEvent<T>(string eventName, TimeSpan timeout, CancellationToken cancellationToken)that combines timeout and external cancellation using linked cancellation token sources - Refactored the timeout-only overload to delegate to the new overload with
CancellationToken.None - Added comprehensive integration tests covering event-wins, cancellation-wins, and timeout-wins scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Abstractions/TaskOrchestrationContext.cs | Added new overload combining timeout and cancellation token functionality; refactored existing timeout overload to delegate to new implementation |
| test/Grpc.IntegrationTests/OrchestrationPatterns.cs | Added three integration tests validating event arrival, external cancellation, and timeout scenarios for the new API |
After thoroughly reviewing this pull request, I found no issues with the implementation. The code is well-written, properly documented following existing patterns, and includes comprehensive test coverage. The implementation correctly uses a linked cancellation token source to coordinate external cancellation with timeout, and the race condition between timer and event is properly handled.
Added test
SummaryWhat changed?
Why is this change needed?Users migrating from in-process mode to isolated mode lost the ability to use Issues / work items
Project checklist
AI-assisted code disclosure (required)Was an AI tool used? (select one)
If AI was used:
AI verification (required if AI was used):
TestingAutomated tests
Manual validation (only if runtime/behavior changed)
Notes for reviewers
|
Here is the PR description formatted according to .github/PULL_REQUEST_TEMPLATE.md: SummaryWhat changed?
Why is this change needed?Users migrating from in-process mode to isolated mode lost the ability to use Issues / work items
Project checklist
AI-assisted code disclosure (required)Was an AI tool used? (select one)
If AI was used:
AI verification (required if AI was used):
TestingAutomated tests
Manual validation (only if runtime/behavior changed)
Notes for reviewers
|
|
related #277 |
Summary
What changed?
Task<T> WaitForExternalEvent<T>(string eventName, TimeSpan timeout, CancellationToken cancellationToken)toTaskOrchestrationContextCancellationToken.NoneWhy is this change needed?
Users migrating from in-process mode to isolated mode lost the ability to use
WaitForExternalEventwith both a timeout and a cancellation token. This prevented implementing race scenarios where multiple events compete with individual timeouts and the winner needs to cancel the losers.Issues / work items
Project checklist
AI-assisted code disclosure (required)
Was an AI tool used? (select one)
If AI was used:
src/Abstractions/TaskOrchestrationContext.cs- New overload implementation and refactoringtest/Grpc.IntegrationTests/OrchestrationPatterns.cs- All 4 integration testsAI verification (required if AI was used):
Testing
Automated tests
Manual validation (only if runtime/behavior changed)
Notes for reviewers
Usage Example
Original prompt
This section details on the original issue you should resolve
<issue_title>WaitForExternalEvent(eventName, timespan, cancellationToken) not supported (anymore?) in isolated mode</issue_title>
<issue_description>Hi there,
Context / issue
We came across an API change in the
WaitForExternalEvent<T>in theMicrosoft.DurableTaskpackage as we had the code below in place for in-process, wait for external events for 7 days, or when the cancellationtoken has ben cancelled when the winner prevails:Now, in isolated mode, we don't have the option to cancel the token externally, and are forced to use a timer to achieve the 'same' functionality as we had in the in-process mode. The only options we have are:

Expected result
To be able to pass in the cancellationtoken next to the timespan as we could in the in-process variant, to cancel the other tasks when one task prevails as winner.
Below some code snippets to demonstrate the issue I try to describe here. Feel free to inform me to clarify or elaborate on certain parts.
Regards,
Tom
Not working:
Not working with timespan and timertask either
Working with timertask: