CmdPal: cancel fetching new items when we get another one (#41166)

This just adds a simple `CancellationToken` around
`ListViewModel.FetchItems()`. Now, when we start a second `FetchItems`
(in responce to a `RaiseItemsChanged`), we'll cancel the old one first.
That'll prevent a particularly long first `GetItems` call from returning
after a second one has already set the list.

Closes #41149

No longer repros the evil sample from #41158
This commit is contained in:
Mike Griese 2025-08-19 06:20:06 -05:00 committed by GitHub
parent fa741470bc
commit 8dbff245d6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

View File

@ -63,6 +63,7 @@ public partial class ListViewModel : PageViewModel, IDisposable
private Task? _initializeItemsTask; private Task? _initializeItemsTask;
private CancellationTokenSource? _cancellationTokenSource; private CancellationTokenSource? _cancellationTokenSource;
private CancellationTokenSource? _fetchItemsCancellationTokenSource;
private ListItemViewModel? _lastSelectedItem; private ListItemViewModel? _lastSelectedItem;
@ -129,14 +130,27 @@ public partial class ListViewModel : PageViewModel, IDisposable
//// Run on background thread, from InitializeAsync or Model_ItemsChanged //// Run on background thread, from InitializeAsync or Model_ItemsChanged
private void FetchItems() private void FetchItems()
{ {
// Cancel any previous FetchItems operation
_fetchItemsCancellationTokenSource?.Cancel();
_fetchItemsCancellationTokenSource?.Dispose();
_fetchItemsCancellationTokenSource = new CancellationTokenSource();
var cancellationToken = _fetchItemsCancellationTokenSource.Token;
// TEMPORARY: just plop all the items into a single group // TEMPORARY: just plop all the items into a single group
// 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;
try try
{ {
// Check for cancellation before starting expensive operations
cancellationToken.ThrowIfCancellationRequested();
var newItems = _model.Unsafe!.GetItems(); var newItems = _model.Unsafe!.GetItems();
// Check for cancellation after getting items from extension
cancellationToken.ThrowIfCancellationRequested();
// Collect all the items into new viewmodels // Collect all the items into new viewmodels
Collection<ListItemViewModel> newViewModels = []; Collection<ListItemViewModel> newViewModels = [];
@ -145,6 +159,9 @@ public partial class ListViewModel : PageViewModel, IDisposable
// building new viewmodels for the ones we haven't already built. // building new viewmodels for the ones we haven't already built.
foreach (var item in newItems) foreach (var item in newItems)
{ {
// Check for cancellation during item processing
cancellationToken.ThrowIfCancellationRequested();
ListItemViewModel viewModel = new(item, new(this)); ListItemViewModel viewModel = new(item, new(this));
// If an item fails to load, silently ignore it. // If an item fails to load, silently ignore it.
@ -154,15 +171,22 @@ public partial class ListViewModel : PageViewModel, IDisposable
} }
} }
// Check for cancellation before initializing first twenty items
cancellationToken.ThrowIfCancellationRequested();
var firstTwenty = newViewModels.Take(20); var firstTwenty = newViewModels.Take(20);
foreach (var item in firstTwenty) foreach (var item in firstTwenty)
{ {
cancellationToken.ThrowIfCancellationRequested();
item?.SafeInitializeProperties(); item?.SafeInitializeProperties();
} }
// Cancel any ongoing search // Cancel any ongoing search
_cancellationTokenSource?.Cancel(); _cancellationTokenSource?.Cancel();
// Check for cancellation before updating the list
cancellationToken.ThrowIfCancellationRequested();
lock (_listLock) lock (_listLock)
{ {
// Now that we have new ViewModels for everything from the // Now that we have new ViewModels for everything from the
@ -173,6 +197,11 @@ public partial class ListViewModel : PageViewModel, IDisposable
// TODO: Iterate over everything in Items, and prune items from the // TODO: Iterate over everything in Items, and prune items from the
// cache if we don't need them anymore // cache if we don't need them anymore
} }
catch (OperationCanceledException)
{
// Cancellation is expected, don't treat as error
return;
}
catch (Exception ex) catch (Exception ex)
{ {
// TODO: Move this within the for loop, so we can catch issues with individual items // TODO: Move this within the for loop, so we can catch issues with individual items
@ -560,6 +589,10 @@ public partial class ListViewModel : PageViewModel, IDisposable
_cancellationTokenSource?.Cancel(); _cancellationTokenSource?.Cancel();
_cancellationTokenSource?.Dispose(); _cancellationTokenSource?.Dispose();
_cancellationTokenSource = null; _cancellationTokenSource = null;
_fetchItemsCancellationTokenSource?.Cancel();
_fetchItemsCancellationTokenSource?.Dispose();
_fetchItemsCancellationTokenSource = null;
} }
protected override void UnsafeCleanup() protected override void UnsafeCleanup()
@ -570,6 +603,7 @@ public partial class ListViewModel : PageViewModel, IDisposable
EmptyContent = new(new(null), PageContext); // necessary? EmptyContent = new(new(null), PageContext); // necessary?
_cancellationTokenSource?.Cancel(); _cancellationTokenSource?.Cancel();
_fetchItemsCancellationTokenSource?.Cancel();
lock (_listLock) lock (_listLock)
{ {