CmdPal: Fix dock band activation lifecycle (#49739)

## Summary of the Pull Request

This PR improve handling of the dock band life cycle, with PerfMon
benefiting from this - it should reduce risk of bands being stuck.

- Remembers the exact IListPage used for the ItemsChanged subscription,
so we have muching unsubscribe.
- Serializes initialization and cleanup to prevent late subscriptions.
- Derives Performance Monitor load state from active subscribers, so our
decisions now follow the real-world state.
- Prevents widget activation counts from underflowing during Dock
rebuilds.
- Adds regression tests for activation transitions and cleanup races.
This commit is contained in:
Jiří Polášek
2026-08-13 00:33:56 +02:00
committed by GitHub
parent 14a966f3ee
commit e453781909
5 changed files with 577 additions and 122 deletions

View File

@@ -4,6 +4,8 @@
using System.Collections.Immutable;
using System.Collections.ObjectModel;
using System.Threading;
using Microsoft.CmdPal.Common.Helpers;
using Microsoft.CmdPal.UI.ViewModels.Models;
using Microsoft.CmdPal.UI.ViewModels.Services;
using Microsoft.CmdPal.UI.ViewModels.Settings;
@@ -19,8 +21,11 @@ public sealed partial class DockBandViewModel : ExtensionObjectViewModel
private readonly CommandItemViewModel _rootItem;
private readonly ISettingsService _settingsService;
private readonly IContextMenuFactory _contextMenuFactory;
private readonly Lock _subscriptionLock = new();
private DockBandSettings _bandSettings;
private InterlockedBoolean _cleanupStarted;
private IListPage? _subscribedList;
public ObservableCollection<DockItemViewModel> Items { get; } = new();
@@ -253,12 +258,26 @@ public sealed partial class DockBandViewModel : ExtensionObjectViewModel
public override void InitializeProperties()
{
if (_cleanupStarted.Value)
{
return;
}
var command = _rootItem.Command;
var list = command.Model.Unsafe as IListPage;
if (list is not null)
{
InitializeFromList(list);
list.ItemsChanged += HandleItemsChanged;
lock (_subscriptionLock)
{
if (_cleanupStarted.Value || _subscribedList is not null)
{
return;
}
list.ItemsChanged += HandleItemsChanged;
_subscribedList = list;
}
}
else
{
@@ -273,6 +292,11 @@ public sealed partial class DockBandViewModel : ExtensionObjectViewModel
private void HandleItemsChanged(object sender, IItemsChangedEventArgs args)
{
if (_cleanupStarted.Value)
{
return;
}
if (_rootItem.Command.Model.Unsafe is IListPage p)
{
InitializeFromList(p);
@@ -281,12 +305,23 @@ public sealed partial class DockBandViewModel : ExtensionObjectViewModel
protected override void UnsafeCleanup()
{
if (!_cleanupStarted.Set())
{
return;
}
base.UnsafeCleanup();
var command = _rootItem.Command;
if (command.Model.Unsafe is IListPage list)
IListPage? subscribedList;
lock (_subscriptionLock)
{
list.ItemsChanged -= HandleItemsChanged;
subscribedList = _subscribedList;
_subscribedList = null;
}
if (subscribedList is not null)
{
subscribedList.ItemsChanged -= HandleItemsChanged;
}
foreach (var item in Items)

View File

@@ -0,0 +1,255 @@
// 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;
using CoreWidgetProvider.Widgets.Enums;
using Microsoft.CommandPalette.Extensions;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Windows.Foundation;
namespace Microsoft.CmdPal.Ext.PerformanceMonitor.UnitTests;
[TestClass]
public partial class PageActivationTests
{
private sealed partial class TrackingPage : OnLoadBasePage
{
public int LoadCount { get; private set; }
public int UnloadCount { get; private set; }
public void TriggerItemsChanged() => RaiseItemsChanged();
protected override void Loaded() => LoadCount++;
protected override void Unloaded() => UnloadCount++;
}
private sealed partial class TrackingWidgetPage : WidgetPage
{
public int ActivationCount { get; private set; }
public int DeactivationCount { get; private set; }
protected override void LoadContentData()
{
}
protected override string GetTemplatePath(WidgetPageState page) => string.Empty;
protected override void OnActivated() => ActivationCount++;
protected override void OnDeactivated() => DeactivationCount++;
}
private sealed partial class ThrowingTrackingPage : OnLoadBasePage
{
public int LoadAttempts { get; private set; }
public int UnloadAttempts { get; private set; }
public int RemainingLoadFailures { get; set; }
public int RemainingUnloadFailures { get; set; }
protected override void Loaded()
{
LoadAttempts++;
if (RemainingLoadFailures > 0)
{
RemainingLoadFailures--;
throw new InvalidOperationException("Load failed.");
}
}
protected override void Unloaded()
{
UnloadAttempts++;
if (RemainingUnloadFailures > 0)
{
RemainingUnloadFailures--;
throw new InvalidOperationException("Unload failed.");
}
}
}
private sealed partial class ThrowingTrackingWidgetPage : WidgetPage
{
public int ActivationAttempts { get; private set; }
public int DeactivationAttempts { get; private set; }
public int RemainingActivationFailures { get; set; }
public int RemainingDeactivationFailures { get; set; }
protected override void LoadContentData()
{
}
protected override string GetTemplatePath(WidgetPageState page) => string.Empty;
protected override void OnActivated()
{
ActivationAttempts++;
if (RemainingActivationFailures > 0)
{
RemainingActivationFailures--;
throw new InvalidOperationException("Activation failed.");
}
}
protected override void OnDeactivated()
{
DeactivationAttempts++;
if (RemainingDeactivationFailures > 0)
{
RemainingDeactivationFailures--;
throw new InvalidOperationException("Deactivation failed.");
}
}
}
[TestMethod]
public void RemovingUnknownHandler_DoesNotUnloadPage()
{
var page = new TrackingPage();
TypedEventHandler<object, IItemsChangedEventArgs> handler = (_, _) => { };
page.ItemsChanged -= handler;
Assert.AreEqual(0, page.LoadCount);
Assert.AreEqual(0, page.UnloadCount);
}
[TestMethod]
public void RemovingDifferentHandler_KeepsPageLoadedAndSubscribed()
{
var page = new TrackingPage();
var notifications = 0;
TypedEventHandler<object, IItemsChangedEventArgs> subscribed = (_, _) => notifications++;
TypedEventHandler<object, IItemsChangedEventArgs> unknown = (_, _) => { };
page.ItemsChanged += subscribed;
page.ItemsChanged -= unknown;
page.TriggerItemsChanged();
Assert.AreEqual(1, page.LoadCount);
Assert.AreEqual(0, page.UnloadCount);
Assert.AreEqual(1, notifications);
page.ItemsChanged -= subscribed;
Assert.AreEqual(1, page.UnloadCount);
}
[TestMethod]
public void DuplicateHandler_UnloadsOnlyAfterFinalRemoval()
{
var page = new TrackingPage();
TypedEventHandler<object, IItemsChangedEventArgs> handler = (_, _) => { };
page.ItemsChanged += handler;
page.ItemsChanged += handler;
page.ItemsChanged -= handler;
Assert.AreEqual(1, page.LoadCount);
Assert.AreEqual(0, page.UnloadCount);
page.ItemsChanged -= handler;
page.ItemsChanged -= handler;
Assert.AreEqual(1, page.LoadCount);
Assert.AreEqual(1, page.UnloadCount);
}
[TestMethod]
public void WidgetActivation_UsesZeroToOneAndOneToZeroTransitions()
{
var page = new TrackingWidgetPage();
page.PopActivate();
page.PushActivate();
page.PushActivate();
page.PopActivate();
Assert.AreEqual(1, page.ActivationCount);
Assert.AreEqual(0, page.DeactivationCount);
page.PopActivate();
page.PopActivate();
Assert.AreEqual(1, page.ActivationCount);
Assert.AreEqual(1, page.DeactivationCount);
page.PushActivate();
Assert.AreEqual(2, page.ActivationCount);
Assert.AreEqual(1, page.DeactivationCount);
}
[TestMethod]
public void LoadFailure_IsContainedAndRetriedOnNextSubscriptionChange()
{
var page = new ThrowingTrackingPage { RemainingLoadFailures = 1 };
TypedEventHandler<object, IItemsChangedEventArgs> first = (_, _) => { };
TypedEventHandler<object, IItemsChangedEventArgs> second = (_, _) => { };
page.ItemsChanged += first;
Assert.AreEqual(1, page.LoadAttempts);
page.ItemsChanged += second;
Assert.AreEqual(2, page.LoadAttempts);
page.ItemsChanged -= first;
Assert.AreEqual(0, page.UnloadAttempts);
page.ItemsChanged -= second;
Assert.AreEqual(1, page.UnloadAttempts);
}
[TestMethod]
public void UnloadFailure_IsContainedAndRetriedOnNextSubscriptionChange()
{
var page = new ThrowingTrackingPage { RemainingUnloadFailures = 1 };
TypedEventHandler<object, IItemsChangedEventArgs> handler = (_, _) => { };
page.ItemsChanged += handler;
page.ItemsChanged -= handler;
Assert.AreEqual(1, page.UnloadAttempts);
page.ItemsChanged -= handler;
Assert.AreEqual(2, page.UnloadAttempts);
}
[TestMethod]
public void ActivationFailure_IsContainedAndRetriedWithoutLosingOwners()
{
var page = new ThrowingTrackingWidgetPage { RemainingActivationFailures = 1 };
page.PushActivate();
Assert.AreEqual(1, page.ActivationAttempts);
page.PushActivate();
Assert.AreEqual(2, page.ActivationAttempts);
page.PopActivate();
Assert.AreEqual(0, page.DeactivationAttempts);
page.PopActivate();
Assert.AreEqual(1, page.DeactivationAttempts);
}
[TestMethod]
public void DeactivationFailure_IsContainedAndRetriedAtZeroOwners()
{
var page = new ThrowingTrackingWidgetPage { RemainingDeactivationFailures = 1 };
page.PushActivate();
page.PopActivate();
Assert.AreEqual(1, page.DeactivationAttempts);
page.PopActivate();
Assert.AreEqual(2, page.DeactivationAttempts);
}
}

View File

@@ -0,0 +1,128 @@
// 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;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CmdPal.UI.ViewModels.Dock;
using Microsoft.CmdPal.UI.ViewModels.Services;
using Microsoft.CmdPal.UI.ViewModels.Settings;
using Microsoft.CommandPalette.Extensions;
using Microsoft.CommandPalette.Extensions.Toolkit;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Moq;
namespace Microsoft.CmdPal.UI.ViewModels.UnitTests;
[TestClass]
public partial class DockBandViewModelLifecycleTests
{
private sealed class TestPageContext : IPageContext
{
public TaskScheduler Scheduler => TaskScheduler.Default;
public ICommandProviderContext ProviderContext => CommandProviderContext.Empty;
public void ShowException(Exception ex, string? extensionHint = null)
{
throw new AssertFailedException($"Unexpected exception from view model: {ex}");
}
}
private sealed partial class BlockingListPage : ListPage, IDisposable
{
private readonly ManualResetEventSlim _getItemsEntered = new();
private readonly ManualResetEventSlim _releaseGetItems = new();
private int _blockNextGetItems;
private int _getItemsCallCount;
public int GetItemsCallCount => Volatile.Read(ref _getItemsCallCount);
public override IListItem[] GetItems()
{
Interlocked.Increment(ref _getItemsCallCount);
if (Interlocked.Exchange(ref _blockNextGetItems, 0) != 0)
{
_getItemsEntered.Set();
if (!_releaseGetItems.Wait(TimeSpan.FromSeconds(5)))
{
throw new TimeoutException("Timed out waiting to resume Dock band initialization.");
}
}
return [];
}
public void BlockNextGetItems()
{
_getItemsEntered.Reset();
_releaseGetItems.Reset();
Interlocked.Exchange(ref _blockNextGetItems, 1);
}
public bool WaitForGetItems() => _getItemsEntered.Wait(TimeSpan.FromSeconds(5));
public void ReleaseGetItems() => _releaseGetItems.Set();
public void TriggerItemsChanged() => RaiseItemsChanged();
public void Dispose()
{
_getItemsEntered.Dispose();
_releaseGetItems.Dispose();
}
}
[TestMethod]
public async Task CleanupDuringInitialization_DoesNotSubscribeAfterCleanup()
{
var context = new TestPageContext();
var page = new BlockingListPage
{
Id = "test.dock.lifecycle",
Name = "Lifecycle test",
Title = "Lifecycle test",
};
var root = new CommandItemViewModel(
new(new CommandItem(page) { Title = page.Title }),
new(context),
DefaultContextMenuFactory.Instance);
root.SlowInitializeProperties();
var settingsService = new Mock<ISettingsService>();
settingsService.SetupGet(service => service.Settings).Returns(new SettingsModel());
var band = new DockBandViewModel(
root,
new(context),
new DockBandSettings { ProviderId = "test", CommandId = page.Id },
settingsService.Object,
DefaultContextMenuFactory.Instance);
try
{
page.BlockNextGetItems();
var initialization = Task.Run(band.InitializeProperties);
Assert.IsTrue(page.WaitForGetItems(), "Dock band initialization did not reach GetItems().");
band.SafeCleanup();
page.ReleaseGetItems();
var completed = await Task.WhenAny(initialization, Task.Delay(TimeSpan.FromSeconds(5)));
Assert.AreSame(initialization, completed, "Dock band initialization did not finish.");
await initialization;
var callsAfterInitialization = page.GetItemsCallCount;
page.TriggerItemsChanged();
Assert.AreEqual(callsAfterInitialization, page.GetItemsCallCount);
}
finally
{
page.ReleaseGetItems();
band.SafeCleanup();
root.SafeCleanup();
page.Dispose();
}
}
}

View File

@@ -4,6 +4,7 @@
using System;
using System.Threading;
using Microsoft.CmdPal.Common;
using Microsoft.CommandPalette.Extensions;
using Microsoft.CommandPalette.Extensions.Toolkit;
using Windows.Foundation;
@@ -72,7 +73,9 @@ internal abstract partial class OnLoadContentPage : OnLoadBasePage, IContentPage
internal abstract partial class OnLoadBasePage : Page
{
private readonly Lock _loadLock = new();
private int _loadCount;
// null means that the last transition failed and the applied state is unknown.
private bool? _isLoaded = false;
#pragma warning disable CS0067 // The event is never used
@@ -83,30 +86,26 @@ internal abstract partial class OnLoadBasePage : Page
{
add
{
InternalItemsChanged += value;
(Exception Exception, bool Loading)? failure;
lock (_loadLock)
{
if (_loadCount == 0)
{
Loaded();
}
_loadCount++;
InternalItemsChanged += value;
failure = ReconcileLoadState();
}
LogTransitionFailure(failure);
}
remove
{
InternalItemsChanged -= value;
(Exception Exception, bool Loading)? failure;
lock (_loadLock)
{
_loadCount--;
_loadCount = Math.Max(0, _loadCount);
if (_loadCount == 0)
{
Unloaded();
}
InternalItemsChanged -= value;
failure = ReconcileLoadState();
}
LogTransitionFailure(failure);
}
}
@@ -116,15 +115,65 @@ internal abstract partial class OnLoadBasePage : Page
protected void RaiseItemsChanged(int totalItems = -1)
{
TypedEventHandler<object, IItemsChangedEventArgs>? handlers;
lock (_loadLock)
{
handlers = InternalItemsChanged;
}
try
{
// TODO #181 - This is the same thing that BaseObservable has to deal with.
InternalItemsChanged?.Invoke(this, new ItemsChangedEventArgs(totalItems));
handlers?.Invoke(this, new ItemsChangedEventArgs(totalItems));
}
catch
{
}
}
private (Exception Exception, bool Loading)? ReconcileLoadState()
{
var shouldBeLoaded = InternalItemsChanged is not null;
if (_isLoaded == shouldBeLoaded)
{
return null;
}
try
{
if (shouldBeLoaded)
{
Loaded();
}
else
{
Unloaded();
}
_isLoaded = shouldBeLoaded;
return null;
}
catch (Exception ex)
{
// The hook may have failed after doing some work. Keep the state
// unknown so the next subscription change reasserts the desired state.
_isLoaded = null;
return (ex, shouldBeLoaded);
}
}
private void LogTransitionFailure((Exception Exception, bool Loading)? failure)
{
if (failure is not { } transitionFailure)
{
return;
}
var state = transitionFailure.Loading ? "loaded" : "unloaded";
CoreLogger.LogError(
$"Failed to transition {GetType().Name} to the {state} state. A later ItemsChanged subscription change will retry the transition.",
transitionFailure.Exception);
}
}

View File

@@ -405,6 +405,12 @@ internal sealed partial class PerformanceWidgetsPage : OnLoadStaticListPage, IDi
/// </summary>
internal abstract partial class WidgetPage : OnLoadContentPage
{
private readonly Lock _activationLock = new();
private int _loadCount;
// null means that the last transition failed and the applied state is unknown.
private bool? _isActive = false;
internal event EventHandler? Updated;
protected Dictionary<string, string> ContentData { get; } = new();
@@ -491,19 +497,85 @@ internal abstract partial class WidgetPage : OnLoadContentPage
/// active. When either is activated, we'll start updating. When both are
/// removed, we'll stop updating.
/// </summary>
internal virtual void PushActivate()
internal void PushActivate()
{
Interlocked.Increment(ref _loadCount);
(Exception Exception, bool Activating)? failure;
lock (_activationLock)
{
_loadCount++;
failure = ReconcileActivation();
}
LogTransitionFailure(failure);
}
internal virtual void PopActivate()
internal void PopActivate()
{
Interlocked.Decrement(ref _loadCount);
(Exception Exception, bool Activating)? failure;
lock (_activationLock)
{
if (_loadCount > 0)
{
_loadCount--;
}
failure = ReconcileActivation();
}
LogTransitionFailure(failure);
}
private int _loadCount;
protected virtual void OnActivated()
{
}
protected bool IsActive => Volatile.Read(ref _loadCount) > 0;
protected virtual void OnDeactivated()
{
}
private (Exception Exception, bool Activating)? ReconcileActivation()
{
var shouldBeActive = _loadCount > 0;
if (_isActive == shouldBeActive)
{
return null;
}
try
{
if (shouldBeActive)
{
OnActivated();
}
else
{
OnDeactivated();
}
_isActive = shouldBeActive;
return null;
}
catch (Exception ex)
{
// The hook may have failed after doing some work. Keep the state
// unknown so the next activation change reasserts the desired state.
_isActive = null;
return (ex, shouldBeActive);
}
}
private void LogTransitionFailure((Exception Exception, bool Activating)? failure)
{
if (failure is not { } transitionFailure)
{
return;
}
var state = transitionFailure.Activating ? "active" : "inactive";
CoreLogger.LogError(
$"Failed to transition performance widget {GetType().Name} to the {state} state. A later activation change will retry the transition.",
transitionFailure.Exception);
}
protected override void Loaded()
{
@@ -605,23 +677,9 @@ internal sealed partial class SystemCPUUsageWidgetPage : WidgetPage, IDisposable
return string.Format(CultureInfo.InvariantCulture, "{0:0.00} GHz", cpuSpeed / 1000);
}
internal override void PushActivate()
{
base.PushActivate();
if (IsActive)
{
_dataManager.Start();
}
}
protected override void OnActivated() => _dataManager.Start();
internal override void PopActivate()
{
base.PopActivate();
if (!IsActive)
{
_dataManager.Stop();
}
}
protected override void OnDeactivated() => _dataManager.Stop();
public void Dispose()
{
@@ -729,23 +787,9 @@ internal sealed partial class SystemMemoryUsageWidgetPage : WidgetPage, IDisposa
return memSize.ToString("0.00", CultureInfo.InvariantCulture) + " GB";
}
internal override void PushActivate()
{
base.PushActivate();
if (IsActive)
{
_dataManager.Start();
}
}
protected override void OnActivated() => _dataManager.Start();
internal override void PopActivate()
{
base.PopActivate();
if (!IsActive)
{
_dataManager.Stop();
}
}
protected override void OnDeactivated() => _dataManager.Stop();
public void Dispose()
{
@@ -869,23 +913,9 @@ internal sealed partial class SystemDiskUsageWidgetPage : WidgetPage, IDisposabl
};
}
internal override void PushActivate()
{
base.PushActivate();
if (IsActive)
{
_dataManager.Start();
}
}
protected override void OnActivated() => _dataManager.Start();
internal override void PopActivate()
{
base.PopActivate();
if (!IsActive)
{
_dataManager.Stop();
}
}
protected override void OnDeactivated() => _dataManager.Stop();
private void HandlePrevDisk()
{
@@ -1058,23 +1088,9 @@ internal sealed partial class SystemNetworkUsageWidgetPage : WidgetPage, IDispos
};
}
internal override void PushActivate()
{
base.PushActivate();
if (IsActive)
{
_dataManager.Start();
}
}
protected override void OnActivated() => _dataManager.Start();
internal override void PopActivate()
{
base.PopActivate();
if (!IsActive)
{
_dataManager.Stop();
}
}
protected override void OnDeactivated() => _dataManager.Stop();
private void HandlePrevNetwork()
{
@@ -1258,23 +1274,9 @@ internal sealed partial class SystemGPUUsageWidgetPage : WidgetPage, IDisposable
return Resources.GetResource("GPU_Usage_Subtitle");
}
internal override void PushActivate()
{
base.PushActivate();
if (IsActive)
{
_dataManager.Start();
}
}
protected override void OnActivated() => _dataManager.Start();
internal override void PopActivate()
{
base.PopActivate();
if (!IsActive)
{
_dataManager.Stop();
}
}
protected override void OnDeactivated() => _dataManager.Stop();
private void HandlePrevGPU()
{
@@ -1467,23 +1469,9 @@ internal sealed partial class SystemBatteryUsageWidgetPage : WidgetPage, IDispos
minutes);
}
internal override void PushActivate()
{
base.PushActivate();
if (IsActive)
{
_dataManager.Start();
}
}
protected override void OnActivated() => _dataManager.Start();
internal override void PopActivate()
{
base.PopActivate();
if (!IsActive)
{
_dataManager.Stop();
}
}
protected override void OnDeactivated() => _dataManager.Stop();
public void Dispose()
{