Fix PeriodicTimer_ActiveOperations_TimerRooted test#68805
Merged
jakobbotsch merged 3 commits intodotnet:mainfrom May 3, 2022
Merged
Fix PeriodicTimer_ActiveOperations_TimerRooted test#68805jakobbotsch merged 3 commits intodotnet:mainfrom
jakobbotsch merged 3 commits intodotnet:mainfrom
Conversation
There are two problems with this test 1. `WaitForNextTickAsync` may return a synchronously completed task, in which case it does not root the timer, causing our first `WaitForTimerToBeCollected` to fail because the timer was collected. This problem is easily reproduced by adding a small sleep after constructing the `PeriodicTimer` in `Create`, and I believe it is the cause of dotnet#59542. 2. There is no guarantee that the timer is not still rooted after the wait finishes because the returned `ValueTask<bool>` may be keeping it alive. This is unlikely since Roslyn should not extend the lifetime of the `ValueTask<bool>` like this across the await, but I have introduced another method just to be safe. Fix dotnet#59542
ghost
added
the
|
Tagging subscribers to this area: @mangod9 Issue DetailsThere are two problems with this test
Fix #59542
|
stephentoub
reviewed
May 3, 2022
src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs
Show resolved
Hide resolved
stephentoub
approved these changes
May 3, 2022
Member
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
One change, otherwise LGTM. Thanks!
…merTests.cs Co-authored-by: Stephen Toub <[email protected]>
danmoseley
reviewed
May 3, 2022
src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs
Outdated
Show resolved
Hide resolved
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
There are two problems with this test
WaitForNextTickAsyncmay return a synchronously completed task, inwhich case it does not root the timer, causing our first
WaitForTimerToBeCollectedto fail because the timer was collected.This problem is easily reproduced by adding a small sleep after
constructing the
PeriodicTimerinCreate, and I believe it is thecause of System.Threading.Tests.PeriodicTimerTests.PeriodicTimer_ActiveOperations_TimerRooted failing in CI #59542.
wait finishes because the returned
ValueTask<bool>may be keepingit alive (although, it seems unlikely Roslyn will extend the lifetime across the
awaitlike this).Fixed by wrapping in another NoInlining method.
Fix #59542