From 7985456939c59e0d5ff812dd8c52eff3d8158a06 Mon Sep 17 00:00:00 2001 From: Michael Hawker <24302614+michael-hawker@users.noreply.github.com> Date: Mon, 25 Nov 2024 16:55:42 -0800 Subject: [PATCH] Switch Debounce DispatcherQueueTimer extension to use ConditionalWeakTable This prevents capture holding onto the timer for garbage collection, validated with a unit test which fails with the ConcurrentDictionary, but succeeds with the ConditionalWeakTable Because of the new Trailing/Leading behavior, we need the table in order to know if something was scheduled, otherwise we'd need reflection if we only stored the event handler, which wouldn't be AOT compatible. --- .../DispatcherQueueTimerExtensions.cs | 15 +++--- .../DispatcherQueueTimerExtensionTests.cs | 50 ++++++++++++++++++- 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/components/Extensions/src/Dispatcher/DispatcherQueueTimerExtensions.cs b/components/Extensions/src/Dispatcher/DispatcherQueueTimerExtensions.cs index 91766926..5204a70d 100644 --- a/components/Extensions/src/Dispatcher/DispatcherQueueTimerExtensions.cs +++ b/components/Extensions/src/Dispatcher/DispatcherQueueTimerExtensions.cs @@ -3,6 +3,8 @@ // See the LICENSE file in the project root for more information. using System.Collections.Concurrent; +using System.Runtime.CompilerServices; + #if WINAPPSDK using DispatcherQueueTimer = Microsoft.UI.Dispatching.DispatcherQueueTimer; @@ -17,8 +19,8 @@ namespace CommunityToolkit.WinUI; /// public static class DispatcherQueueTimerExtensions { - // TODO: We should use a WeakReference here... - private static ConcurrentDictionary _debounceInstances = new ConcurrentDictionary(); + //// https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2 + private static ConditionalWeakTable _debounceInstances = new(); /// /// Used to debounce (rate-limit) an event. The action will be postponed and executed after the interval has elapsed. At the end of the interval, the function will be called with the arguments that were passed most recently to the debounced function. Useful for smoothing keyboard input, for instance. @@ -60,11 +62,11 @@ public static void Debounce(this DispatcherQueueTimer timer, Action action, Time { // If we have a _debounceInstance queued, then we were running in trailing mode, // so if we now have the immediate flag, we should ignore this timer, and run immediately. - if (_debounceInstances.ContainsKey(timer)) + if (_debounceInstances.TryGetValue(timer, out var _)) { timeout = false; - _debounceInstances.Remove(timer, out var _); + _debounceInstances.Remove(timer); } // If we're in immediate mode then we only execute if the timer wasn't running beforehand @@ -79,7 +81,7 @@ public static void Debounce(this DispatcherQueueTimer timer, Action action, Time timer.Tick += Timer_Tick; // Store/Update function - _debounceInstances.AddOrUpdate(timer, action, (k, v) => action); + _debounceInstances.AddOrUpdate(timer, action); } // Start the timer to keep track of the last call here. @@ -94,8 +96,9 @@ private static void Timer_Tick(object sender, object e) timer.Tick -= Timer_Tick; timer.Stop(); - if (_debounceInstances.TryRemove(timer, out Action? action)) + if (_debounceInstances.TryGetValue(timer, out Action? action)) { + _debounceInstances.Remove(timer); action?.Invoke(); } } diff --git a/components/Extensions/tests/DispatcherQueueTimerExtensionTests.cs b/components/Extensions/tests/DispatcherQueueTimerExtensionTests.cs index 8243fbfb..7b636572 100644 --- a/components/Extensions/tests/DispatcherQueueTimerExtensionTests.cs +++ b/components/Extensions/tests/DispatcherQueueTimerExtensionTests.cs @@ -4,7 +4,6 @@ using CommunityToolkit.Tests; using CommunityToolkit.Tooling.TestGen; -using CommunityToolkit.WinUI; #if WINAPPSDK using DispatcherQueue = Microsoft.UI.Dispatching.DispatcherQueue; @@ -327,4 +326,53 @@ public async Task DispatcherQueueTimer_Debounce_Leading_Switch_Trailing_Interrup Assert.AreEqual(value3, triggeredValue, "Expected value to now be the last value provided."); Assert.AreEqual(2, triggeredCount, "Expected to interrupt execution of 2nd request."); } + + [TestCategory("DispatcherQueueTimerExtensions")] + [UIThreadTestMethod] + public async Task DispatcherQueueTimer_Debounce_Trailing_Stop_Lifetime() + { + // Our test indicator + WeakReference? reference = null; + + // Still need to capture this on our UI thread + DispatcherQueue _queue = DispatcherQueue.GetForCurrentThread(); + + await Task.Run(() => + { + // This test checks the lifetime of the timer and if we hold a reference to it. + var debounceTimer = _queue.CreateTimer(); + + // Track the DispatcherQueueTimer object + reference = new WeakReference(debounceTimer, true); + + var triggeredCount = 0; + string? triggeredValue = null; + + var value = "He"; + debounceTimer.Debounce( + () => + { + triggeredCount++; + triggeredValue = value; + }, + TimeSpan.FromMilliseconds(60)); + + // Stop the timer before it would fire, with our proper method to clean-up + debounceTimer.Stop(); + + Assert.AreEqual(false, debounceTimer.IsRunning, "Expected to stop the timer."); + + debounceTimer = null; + }); + + // Now out of scope and see if GC cleans up + GC.Collect(); + GC.WaitForPendingFinalizers(); + + // Clean-up any UI thread work + await CompositionTargetHelper.ExecuteAfterCompositionRenderingAsync(() => { }); + + Assert.IsNotNull(reference, "Didn't capture weak reference."); + Assert.IsNull(reference.Target, "Strong reference to DispatcherQueueTimer still exists."); + } }