CmdPal: Clean up ListItemViewModels when we no longer need them (#41169)
Some checks failed
Spell checking / Check Spelling (push) Has been cancelled
Spell checking / Report (Push) (push) Has been cancelled
Spell checking / Report (PR) (push) Has been cancelled
Spell checking / Update PR (push) Has been cancelled

_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.
This commit is contained in:
Mike Griese 2025-08-19 16:02:38 -05:00 committed by GitHub
parent 917da2e07e
commit ce4d8dc11e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 46 additions and 4 deletions

View File

@ -141,6 +141,9 @@ public partial class ListViewModel : PageViewModel, IDisposable
// see 9806fe5d8 for the last commit that had this with sections // see 9806fe5d8 for the last commit that had this with sections
_isFetching = true; _isFetching = true;
// Collect all the items into new viewmodels
Collection<ListItemViewModel> newViewModels = [];
try try
{ {
// Check for cancellation before starting expensive operations // 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 // Check for cancellation after getting items from extension
cancellationToken.ThrowIfCancellationRequested(); cancellationToken.ThrowIfCancellationRequested();
// Collect all the items into new viewmodels
Collection<ListItemViewModel> newViewModels = [];
// TODO we can probably further optimize this by also keeping a // TODO we can probably further optimize this by also keeping a
// HashSet of every ExtensionObject we currently have, and only // HashSet of every ExtensionObject we currently have, and only
// building new viewmodels for the ones we haven't already built. // 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 // Check for cancellation before updating the list
cancellationToken.ThrowIfCancellationRequested(); cancellationToken.ThrowIfCancellationRequested();
List<ListItemViewModel> removedItems = [];
lock (_listLock) lock (_listLock)
{ {
// Now that we have new ViewModels for everything from the // Now that we have new ViewModels for everything from the
// extension, smartly update our list of VMs // 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 // TODO: Iterate over everything in Items, and prune items from the
@ -200,6 +211,15 @@ public partial class ListViewModel : PageViewModel, IDisposable
catch (OperationCanceledException) catch (OperationCanceledException)
{ {
// Cancellation is expected, don't treat as error // 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; return;
} }
catch (Exception ex) catch (Exception ex)

View File

@ -65,12 +65,32 @@ public partial class ListHelpers
public static void InPlaceUpdateList<T>(IList<T> original, IEnumerable<T> newContents) public static void InPlaceUpdateList<T>(IList<T> original, IEnumerable<T> newContents)
where T : class where T : class
{ {
InPlaceUpdateList(original, newContents, out _);
}
/// <summary>
/// Modifies the contents of `original` in-place, to match those of
/// `newContents`. The canonical use being:
/// ```cs
/// ListHelpers.InPlaceUpdateList(FilteredItems, FilterList(ItemsToFilter, TextToFilterOn));
/// ```
/// </summary>
/// <typeparam name="T">Any type that can be compared for equality</typeparam>
/// <param name="original">Collection to modify</param>
/// <param name="newContents">The enumerable which `original` should match</param>
/// <param name="removedItems">List of items that were removed from the original collection</param>
public static void InPlaceUpdateList<T>(IList<T> original, IEnumerable<T> newContents, out List<T> removedItems)
where T : class
{
removedItems = [];
// we're not changing newContents - stash this so we don't re-evaluate it every time // we're not changing newContents - stash this so we don't re-evaluate it every time
var numberOfNew = newContents.Count(); var numberOfNew = newContents.Count();
// Short circuit - new contents should just be empty // Short circuit - new contents should just be empty
if (numberOfNew == 0) if (numberOfNew == 0)
{ {
removedItems.AddRange(original);
original.Clear(); original.Clear();
return; return;
} }
@ -92,6 +112,7 @@ public partial class ListHelpers
for (var k = i; k < j; k++) for (var k = i; k < j; k++)
{ {
// This item from the original list was not in the new list. Remove it. // This item from the original list was not in the new list. Remove it.
removedItems.Add(original[i]);
original.RemoveAt(i); original.RemoveAt(i);
} }
@ -120,6 +141,7 @@ public partial class ListHelpers
while (original.Count > numberOfNew) while (original.Count > numberOfNew)
{ {
// RemoveAtEnd // RemoveAtEnd
removedItems.Add(original[original.Count - 1]);
original.RemoveAt(original.Count - 1); original.RemoveAt(original.Count - 1);
} }