From ce4d8dc11ee7c22de37314b022c9c78dfae7aeca Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 19 Aug 2025 16:02:38 -0500 Subject: [PATCH] CmdPal: Clean up ListItemViewModels when we no longer need them (#41169) _We already fixed one leak, yes, but what about second leak?_ We already clean up `ListItemViewModel`s for a page when the page is navigated away from. However, if the page updates it's items, we would never actually `Cleanup` the old items. We'd just lose them, and never unregister their event handlers. The objects would just leak forever. This builds on the work in #41166, to do two things: * Cleanup items that were removed from our list, when we actually update `Items`. This involved a change to `Toolkit.ListHelpers`, to let us know which items were removed from the list during `InPlaceUpdateList` * Cleanup items that are thrown out when we cancel a FetchItems. Those items were constructed, and might have registered event handlers, even if we never actually put them into `Items`. _Targets #41166_ Closes #39837 Tested with the evil sample from #41158, and loading thousands and thousands of items no longer causes us to leak memory like we're Deepwater Horizon. --- .../ListViewModel.cs | 28 ++++++++++++++++--- .../ListHelpers.cs | 22 +++++++++++++++ 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/ListViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/ListViewModel.cs index c461943f8a..10fc9445ad 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/ListViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/ListViewModel.cs @@ -141,6 +141,9 @@ public partial class ListViewModel : PageViewModel, IDisposable // see 9806fe5d8 for the last commit that had this with sections _isFetching = true; + // Collect all the items into new viewmodels + Collection newViewModels = []; + try { // Check for cancellation before starting expensive operations @@ -151,9 +154,6 @@ public partial class ListViewModel : PageViewModel, IDisposable // Check for cancellation after getting items from extension cancellationToken.ThrowIfCancellationRequested(); - // Collect all the items into new viewmodels - Collection newViewModels = []; - // TODO we can probably further optimize this by also keeping a // HashSet of every ExtensionObject we currently have, and only // building new viewmodels for the ones we haven't already built. @@ -187,11 +187,22 @@ public partial class ListViewModel : PageViewModel, IDisposable // Check for cancellation before updating the list cancellationToken.ThrowIfCancellationRequested(); + List removedItems = []; lock (_listLock) { // Now that we have new ViewModels for everything from the // extension, smartly update our list of VMs - ListHelpers.InPlaceUpdateList(Items, newViewModels); + ListHelpers.InPlaceUpdateList(Items, newViewModels, out removedItems); + + // DO NOT ThrowIfCancellationRequested AFTER THIS! If you do, + // you'll clean up list items that we've now transferred into + // .Items + } + + // If we removed items, we need to clean them up, to remove our event handlers + foreach (var removedItem in removedItems) + { + removedItem.SafeCleanup(); } // TODO: Iterate over everything in Items, and prune items from the @@ -200,6 +211,15 @@ public partial class ListViewModel : PageViewModel, IDisposable catch (OperationCanceledException) { // Cancellation is expected, don't treat as error + + // However, if we were cancelled, we didn't actually add these items to + // our Items list. Before we release them to the GC, make sure we clean + // them up + foreach (var vm in newViewModels) + { + vm.SafeCleanup(); + } + return; } catch (Exception ex) diff --git a/src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/ListHelpers.cs b/src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/ListHelpers.cs index 4f004ae78e..39f7c087b0 100644 --- a/src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/ListHelpers.cs +++ b/src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/ListHelpers.cs @@ -65,12 +65,32 @@ public partial class ListHelpers public static void InPlaceUpdateList(IList original, IEnumerable newContents) where T : class { + InPlaceUpdateList(original, newContents, out _); + } + + /// + /// Modifies the contents of `original` in-place, to match those of + /// `newContents`. The canonical use being: + /// ```cs + /// ListHelpers.InPlaceUpdateList(FilteredItems, FilterList(ItemsToFilter, TextToFilterOn)); + /// ``` + /// + /// Any type that can be compared for equality + /// Collection to modify + /// The enumerable which `original` should match + /// List of items that were removed from the original collection + public static void InPlaceUpdateList(IList original, IEnumerable newContents, out List removedItems) + where T : class + { + removedItems = []; + // we're not changing newContents - stash this so we don't re-evaluate it every time var numberOfNew = newContents.Count(); // Short circuit - new contents should just be empty if (numberOfNew == 0) { + removedItems.AddRange(original); original.Clear(); return; } @@ -92,6 +112,7 @@ public partial class ListHelpers for (var k = i; k < j; k++) { // This item from the original list was not in the new list. Remove it. + removedItems.Add(original[i]); original.RemoveAt(i); } @@ -120,6 +141,7 @@ public partial class ListHelpers while (original.Count > numberOfNew) { // RemoveAtEnd + removedItems.Add(original[original.Count - 1]); original.RemoveAt(original.Count - 1); }