-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[release/9.0] Check CancellationToken before TimeSpan in TimeProviderTaskExtensions #107416
Conversation
…sions This makes the down-level behavior for Delay and WaitAsync match the behavior in .NET 8 and later, prefering to return a Canceled task over a Completed task or TimeoutException when the TimeSpan is Zero and the CancellationToken is already Canceled. Fixes #106996
Tagging subscribers to this area: @dotnet/area-system-datetime |
What's the motivation for getting this into .NET 9? It's pretty minor, and pre-existing, no? |
It was reported by someone who ran into this issue, and I am not aware of any workaround. Do you have a good suggestion for what to do for those who run into it? do you see any risk taking this change? |
FWIW, I found this bug while trying to integrate |
Just check token.IsCancellationRequested, either directly or indirectly via ThrowIfCancellationRequested.
No, it's low risk. We're just at a point in the cycle where there generally needs to be a strong justification for any churn. |
I was viewing this as a compat issue in a recently introduced feature which is why I brought it into 9.0. |
Ok, it's up to you. Generally though this is a non-deterministic scenario, with a race with cancellation being requested, so I don't see this as being particularly impactful. |
Backport of #107009 to release/9.0
/cc @tarekgh @brantburnett
Customer Impact
[Select one or both of the boxes. Describe how this issue impacts customers, citing the expected and actual behaviors and scope of the issue. If customer-reported, provide the issue number.]
Users utilizing
TimeProvider
and callingDelay
orWaitAsync
may encounter inconsistent task cancellation behavior between .NET 8.0+ and earlier versions. In .NET 8.0 and later, the task status will becanceled
, while on down-level versions, the task could be marked ascompleted
if the time expires, even if it was canceled. This discrepancy can lead to apps behaving differently depending on the .NET version they're running on.Regression
[If yes, specify when the regression was introduced. Provide the PR or commit if known.]
Testing
[How was the fix verified? How was the issue missed previously? What tests were added?]
Added new test to catch the failing issue and passed all regression tests too.
Risk
[High/Medium/Low. Justify the indication by mentioning how risks were measured and addressed.]
Low, there is no real change more than switching the order task status checking before completing the task. we used to check the time expiration then we check the cancellation. Now, we check cancellation first and then we check the time expiration. The change only affect apps when running on down-levels. i.e. shouldn't affect apps running on .NET 8 or 9
IMPORTANT: If this backport is for a servicing release, please verify that:
The PR target branch is
release/X.0-staging
, notrelease/X.0
.If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.