From 6110ab22870762cb84b25eadb3bc8a7cf201f220 Mon Sep 17 00:00:00 2001
From: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
Date: Tue, 3 May 2022 19:47:09 +0200
Subject: [PATCH] Fix PeriodicTimer_ActiveOperations_TimerRooted test (#68805)

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
---
 .../System/Threading/PeriodicTimerTests.cs    | 42 +++++++++++++++----
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs b/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs
index 099096ac4f540..05cdd8e922228 100644
--- a/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs
+++ b/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs
@@ -112,20 +112,46 @@ static WeakReference<PeriodicTimer> Create() =>
         [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 things in 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);
+                    }
+
+                    task.GetAwaiter().GetResult();
+
+                    waitMs *= 2;
+                }
+
+                throw new Exception("Expected to be able to create an active wait for a timer");
             }
         }