-
-
Notifications
You must be signed in to change notification settings - Fork 108
perf: use Task.WaitAsync for efficient cancellation in TimeoutHelper #4289
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
Co-Authored-By: Claude Opus 4.5 <[email protected]>
SummaryOptimizes TimeoutHelper to use Critical IssuesThis PR doesn't implement the optimization described in issue #4287. Issue #4287 correctly identifies that when no timeout is configured, we don't need to "race" against cancellation at all. The suggested fix is to simply await the task directly: if (!timeout.HasValue)
{
return await taskFactory(cancellationToken).ConfigureAwait(false);
}Why this matters:
Recommendation: Remove the entire if (!timeout.HasValue)
{
return await taskFactory(cancellationToken).ConfigureAwait(false);
}This achieves the goal stated in #4287: "Reduces per-test overhead by ~5% for tests without timeouts" by eliminating all the allocation overhead, not just reducing it. SuggestionsThe .gitignore change (adding Verdict |
Remove unnecessary cancellation racing infrastructure when no timeout is specified. The task factory already receives the cancellation token, so cancellation is handled cooperatively by the task itself. This eliminates all TCS, Register, and WhenAny allocations in the no-timeout code path. Fixes #4287 Co-Authored-By: Claude Opus 4.5 <[email protected]>
d0a0e0e to
5209dde
Compare
|
Addressed review feedback:
|
SummaryThis PR simplifies the no-timeout fast path in TimeoutHelper by removing the TaskCompletionSource racing pattern and directly awaiting the task. Critical Issues1. Semantic Change in Cancellation BehaviorThe PR changes cancellation semantics in a potentially breaking way. With the OLD code, when cancellationToken fires, the await completes immediately by racing against a TCS. With the NEW code, the await only completes when the task itself checks the token and throws OperationCanceledException. This could cause tests to not cancel promptly when the engine requests cancellation, especially if the task is in a long-running synchronous operation or awaiting something that doesn't respect cancellation. 2. Misleading PR DescriptionThe PR description states it uses Task.WaitAsync(CancellationToken) on .NET 6+, but the actual code does NOT use Task.WaitAsync at all - it just does a direct await. SuggestionsUse Task.WaitAsync to preserve semantics while improving performance. This would eliminate TCS allocations on .NET 8+ while preserving cancellation racing behavior and matching the PR description. VerdictREQUEST CHANGES - The current implementation changes cancellation semantics. Consider using Task.WaitAsync to achieve the performance goal while preserving existing behavior. |
Address review feedback by using Task.WaitAsync(CancellationToken) on .NET 6+ to preserve the "stop waiting immediately on cancellation" semantics while eliminating TCS allocations. Co-Authored-By: Claude Opus 4.5 <[email protected]>
SummaryThis PR optimizes TimeoutHelper to use Task.WaitAsync(CancellationToken) on .NET 6+ to avoid TCS/registration allocations when no timeout is specified. Critical IssuesBehavioral regression on pre-.NET 6 frameworks: The fallback path for .NET Framework/.NET Standard 2.0 (TUnit.Engine/Helpers/TimeoutHelper.cs:69-71) changes cancellation behavior by removing immediate-return-on-cancellation. OLD BEHAVIOR: When cancellationToken is cancelled, control returns immediately (the TCS task wins the race) This violates the documented contract and could cause tests to hang on older frameworks if the task doesn't frequently check the token. FIX: Keep the TCS+WhenAny pattern for pre-.NET 6 frameworks to preserve immediate cancellation behavior, or explicitly document that immediate cancellation only works on .NET 6+. SuggestionsThe .NET 6+ path looks good - WaitAsync is exactly the right tool for this optimization. VerdictREQUEST CHANGES - Pre-.NET 6 behavioral regression must be addressed |
Summary
Task.WaitAsync(CancellationToken)on .NET 6+ for efficient cancellation handling when no timeout is specifiedFixes #4287
Test plan
🤖 Generated with Claude Code