From e0f62ccdc6f9b3ee2f268583a349c54025c81454 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 3 May 2022 12:41:39 +0200 Subject: [PATCH 1/3] Fix PeriodicTimer_ActiveOperations_TimerRooted test 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` may be keeping it alive. This is unlikely since Roslyn should not extend the lifetime of the `ValueTask` like this across the await, but I have introduced another method just to be safe. Fix #59542 --- .../System/Threading/PeriodicTimerTests.cs | 40 +++++++++++++++---- 1 file changed, 32 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 099096ac4f5406..518a36c6bf79ba 100644 --- a/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs +++ b/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs @@ -112,20 +112,44 @@ static WeakReference Create() => [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))] public async Task PeriodicTimer_ActiveOperations_TimerRooted() { - (WeakReference timer, ValueTask task) = Create(); + // Step 1: Verify that if we have an active wait the timer does not get collected. + WeakReference 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> CreateAndVerifyRooted() + { + (WeakReference timer, ValueTask task) = CreateActive(); - WaitForTimerToBeCollected(timer, expected: true); + WaitForTimerToBeCollected(timer, expected: false); + + Assert.True(await task); + + return timer; + } [MethodImpl(MethodImplOptions.NoInlining)] - static (WeakReference, ValueTask) Create() + static (WeakReference, ValueTask) CreateActive() { - var timer = new PeriodicTimer(TimeSpan.FromMilliseconds(1)); - ValueTask task = timer.WaitForNextTickAsync(); - return (new WeakReference(timer), task); + int waitMs = 1; + for (int i = 0; i < 10; i++) + { + var timer = new PeriodicTimer(TimeSpan.FromMilliseconds(waitMs)); + ValueTask task = timer.WaitForNextTickAsync(); + if (!task.IsCompleted) + { + return (new WeakReference(timer), task); + } + + waitMs *= 2; + } + + throw new Exception("Expected to be able to create an active wait for a timer"); } } From de3c1d87d57b5719bf6cdda29411173199008c8e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 3 May 2022 16:01:51 +0200 Subject: [PATCH 2/3] Update src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs Co-authored-by: Stephen Toub --- .../System.Runtime/tests/System/Threading/PeriodicTimerTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs b/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs index 518a36c6bf79ba..fcaa7ec156e976 100644 --- a/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs +++ b/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs @@ -146,6 +146,8 @@ static async ValueTask> CreateAndVerifyRooted() return (new WeakReference(timer), task); } + task.GetAwaiter().GetResult(); + waitMs *= 2; } From 981eb63731f04e5faf60c8a2102176ffe3721217 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 3 May 2022 16:06:20 +0200 Subject: [PATCH 3/3] Fix typo --- .../System.Runtime/tests/System/Threading/PeriodicTimerTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs b/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs index fcaa7ec156e976..05cdd8e9222286 100644 --- a/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs +++ b/src/libraries/System.Runtime/tests/System/Threading/PeriodicTimerTests.cs @@ -118,7 +118,7 @@ public async Task PeriodicTimer_ActiveOperations_TimerRooted() // Step 2: Verify that now the timer does get collected WaitForTimerToBeCollected(timer, expected: true); - // It is important that we do these two thing sin NoInlining + // 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)]