System.Threading.Tests.PeriodicTimerTests.PeriodicTimer_ActiveOperations_TimerRooted failing in CI #59542
Comments
|
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
dotnet-issue-labeler
bot
added
the
untriaged
jkoritzinsky
added
blocking-clean-ci
|
Tagging subscribers to this area: @mangod9 Issue DetailsRunfo Creating Tracking Issue (data being generated)
|
|
This is very likely another case of us just not being able to test reliably that things are becoming unrooted and available for collection, e.g. |
|
@stephentoub do you suggest to disable the test similarly as was done in #57751? |
|
Yeah, I think that makes sense, until we can figure out how to better stabilize these. |
|
Do we have any idea how? Can GC folks indicate whether it’s even possible? |
|
This has failed again on the first attempt here: https://dev.azure.com/dnceng/public/_build/results?buildId=1440422&view=results - I'm not sure why runfo hasn't updated |
|
@mangod9 can GC folks advise on whether we should expect these patterns to be deterministic? In this case, it wants to verify a weak reference was collected private static void WaitForTimerToBeCollected(WeakReference<PeriodicTimer> timer, bool expected)
{
Assert.Equal(expected, SpinWait.SpinUntil(() =>
{
GC.Collect();
return !timer.TryGetTarget(out _);
}, TimeSpan.FromSeconds(expected ? 5 : 0.5)));
}In the issue @stephentoub linked above, it's verifying a finalization happened, with: for (int i = 0; i < 10 && !Volatile.Read(ref finalized); i++)
{
GC.Collect();
GC.WaitForPendingFinalizers();
await Task.Yield();
}Patterns like this happen in a number of places in our tests, all assuming that GC.Collect() and GC.WaitForPendingFinalizers() are deterministic if we loop them "a few times" - it would be good to know what we can rely on. |
|
@cshung can you advise on q above? |
|
if you are calling GC.Collect(), GC is not in charge of the lifetime. please see this comment. |
|
OK, we need to audit all our tests that do GC.Collect(), and ensure that (or refactor such that):
|
|
@Danyy427 another one perhaps |
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
in-pr
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 #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 (although, it seems unlikely Roslyn will extend the lifetime across the await like this). Fixed by wrapping in another NoInlining method. Fix #59542
ghost
removed
the
in-pr
ghost
locked as resolved and limited conversation to collaborators
Runfo Tracking Issue: System.Threading.Tests.PeriodicTimerTests.PeriodicTimer_ActiveOperations_TimerRooted failing in CI
Build Result Summary
The text was updated successfully, but these errors were encountered: