Skip to content

Commit

Permalink
Fix PeriodicTimer_ActiveOperations_TimerRooted test
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jakobbotsch committed May 3, 2022
1 parent 288de11 commit e0f62cc
Showing 1 changed file with 32 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -112,20 +112,44 @@ public void PeriodicTimer_NoActiveOperations_TimerNotRooted()
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
public async Task PeriodicTimer_ActiveOperations_TimerRooted()
{
(WeakReference<PeriodicTimer> timer, ValueTask<bool> task) = Create();
// Step 1: Verify that if we have an active wait the timer does not get collected.
WeakReference<PeriodicTimer> timer = await CreateAndVerifyRooted();

WaitForTimerToBeCollected(timer, expected: false);
// Step 2: Verify that now the timer does get collected
WaitForTimerToBeCollected(timer, expected: true);

Assert.True(await task);
// It is important that we do these two thing sin NoInlining
// methods. We are only guaranteed that references inside these
// methods are not live anymore when the functions return.
[MethodImpl(MethodImplOptions.NoInlining)]
static async ValueTask<WeakReference<PeriodicTimer>> CreateAndVerifyRooted()
{
(WeakReference<PeriodicTimer> timer, ValueTask<bool> task) = CreateActive();

WaitForTimerToBeCollected(timer, expected: true);
WaitForTimerToBeCollected(timer, expected: false);

Assert.True(await task);

return timer;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static (WeakReference<PeriodicTimer>, ValueTask<bool>) Create()
static (WeakReference<PeriodicTimer>, ValueTask<bool>) CreateActive()
{
var timer = new PeriodicTimer(TimeSpan.FromMilliseconds(1));
ValueTask<bool> task = timer.WaitForNextTickAsync();
return (new WeakReference<PeriodicTimer>(timer), task);
int waitMs = 1;
for (int i = 0; i < 10; i++)
{
var timer = new PeriodicTimer(TimeSpan.FromMilliseconds(waitMs));
ValueTask<bool> task = timer.WaitForNextTickAsync();
if (!task.IsCompleted)
{
return (new WeakReference<PeriodicTimer>(timer), task);
}

waitMs *= 2;
}

throw new Exception("Expected to be able to create an active wait for a timer");
}
}

Expand Down

0 comments on commit e0f62cc

Please sign in to comment.