mirror of
https://github.com/microsoft/PowerToys
synced 2025-08-22 01:58:04 +00:00
CmdPal: Avoid reentrancy issues when loading more items (#40715)
## Summary of the Pull Request When checking the HasMoreItems flag, COM can start a nested message pump, which allows a reentrant call on the XAML UI and causes a fast fail. This change moves the check off the UI thread to prevent reentrancy, but the loading flag is set before we know for sure that there is something to load. This update also introduces a change: if LoadMore fails, we clear the loading flag immediately. ## PR Checklist - [x] **Closes:** #40707 - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [ ] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed
This commit is contained in:
parent
114c3972be
commit
498fe75c4a
@ -0,0 +1,46 @@
|
||||
// Copyright (c) Microsoft Corporation
|
||||
// The Microsoft Corporation licenses this file to you under the MIT license.
|
||||
// See the LICENSE file in the project root for more information.
|
||||
|
||||
using System.Threading;
|
||||
|
||||
namespace Microsoft.CmdPal.Common.Helpers;
|
||||
|
||||
/// <summary>
|
||||
/// Thread-safe boolean implementation using atomic operations
|
||||
/// </summary>
|
||||
public struct InterlockedBoolean(bool initialValue = false)
|
||||
{
|
||||
private int _value = initialValue ? 1 : 0;
|
||||
|
||||
/// <summary>
|
||||
/// Gets or sets the boolean value atomically
|
||||
/// </summary>
|
||||
public bool Value
|
||||
{
|
||||
get => Volatile.Read(ref _value) == 1;
|
||||
set => Interlocked.Exchange(ref _value, value ? 1 : 0);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Atomically sets the value to true
|
||||
/// </summary>
|
||||
/// <returns>True if the value was previously false, false if it was already true</returns>
|
||||
public bool Set()
|
||||
{
|
||||
return Interlocked.Exchange(ref _value, 1) == 0;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Atomically sets the value to false
|
||||
/// </summary>
|
||||
/// <returns>True if the value was previously true, false if it was already false</returns>
|
||||
public bool Clear()
|
||||
{
|
||||
return Interlocked.Exchange(ref _value, 0) == 1;
|
||||
}
|
||||
|
||||
public override int GetHashCode() => Value.GetHashCode();
|
||||
|
||||
public override string ToString() => Value.ToString();
|
||||
}
|
@ -6,6 +6,7 @@ using System.Collections.ObjectModel;
|
||||
using CommunityToolkit.Mvvm.ComponentModel;
|
||||
using CommunityToolkit.Mvvm.Input;
|
||||
using CommunityToolkit.Mvvm.Messaging;
|
||||
using Microsoft.CmdPal.Common.Helpers;
|
||||
using Microsoft.CmdPal.Core.ViewModels.Messages;
|
||||
using Microsoft.CmdPal.Core.ViewModels.Models;
|
||||
using Microsoft.CommandPalette.Extensions;
|
||||
@ -31,7 +32,7 @@ public partial class ListViewModel : PageViewModel, IDisposable
|
||||
|
||||
private readonly Lock _listLock = new();
|
||||
|
||||
private bool _isLoading;
|
||||
private InterlockedBoolean _isLoading;
|
||||
private bool _isFetching;
|
||||
|
||||
public event TypedEventHandler<ListViewModel, object>? ItemsUpdated;
|
||||
@ -121,7 +122,7 @@ public partial class ListViewModel : PageViewModel, IDisposable
|
||||
|
||||
ItemsUpdated?.Invoke(this, EventArgs.Empty);
|
||||
UpdateEmptyContent();
|
||||
_isLoading = false;
|
||||
_isLoading.Clear();
|
||||
}
|
||||
}
|
||||
|
||||
@ -221,7 +222,7 @@ public partial class ListViewModel : PageViewModel, IDisposable
|
||||
}
|
||||
|
||||
ItemsUpdated?.Invoke(this, EventArgs.Empty);
|
||||
_isLoading = false;
|
||||
_isLoading.Clear();
|
||||
});
|
||||
}
|
||||
|
||||
@ -469,21 +470,38 @@ public partial class ListViewModel : PageViewModel, IDisposable
|
||||
return;
|
||||
}
|
||||
|
||||
if (model.HasMoreItems && !_isLoading)
|
||||
if (!_isLoading.Set())
|
||||
{
|
||||
_isLoading = true;
|
||||
_ = Task.Run(() =>
|
||||
return;
|
||||
|
||||
// NOTE: May miss newly available items until next scroll if model
|
||||
// state changes between our check and this reset
|
||||
}
|
||||
|
||||
_ = Task.Run(() =>
|
||||
{
|
||||
// Execute all COM calls on background thread to avoid reentrancy issues with UI
|
||||
// with the UI thread when COM starts inner message pump
|
||||
try
|
||||
{
|
||||
try
|
||||
if (model.HasMoreItems)
|
||||
{
|
||||
model.LoadMore();
|
||||
|
||||
// _isLoading flag will be set as a result of LoadMore,
|
||||
// which must raise ItemsChanged to end the loading.
|
||||
}
|
||||
catch (Exception ex)
|
||||
else
|
||||
{
|
||||
ShowException(ex, model.Name);
|
||||
_isLoading.Clear();
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
_isLoading.Clear();
|
||||
ShowException(ex, model.Name);
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
protected override void FetchProperty(string propertyName)
|
||||
|
Loading…
x
Reference in New Issue
Block a user