Fix code review: move SaveImageSizes outside lock, improve thread safety in Dispose

Agent-Logs-Url: https://github.com/microsoft/PowerToys/sessions/7e1a7335-defa-4fd6-8eff-7e72b75bece6

Co-authored-by: MuyuanMS <116717757+MuyuanMS@users.noreply.github.com>

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/AliasManager.cs

@@ -52,15 +52,6 @@ public partial class AliasManager : ObservableObject
                     WeakReferenceMessenger.Default.Send<PerformCommandMessage>(topLevelCommand.GetPerformCommandMessage());
                     return true;
                 }
-                else if (_topLevelCommandManager.HasFinishedLoadingCommands)
-                {
-                    // The command no longer exists (e.g. extension uninstalled or command renamed).
-                    // Only remove the orphaned alias once extension loading has fully settled,
-                    // otherwise valid aliases can be deleted while commands are still arriving.
-                    _settingsService.UpdateSettings(
-                        s => s with { Aliases = s.Aliases.Remove(searchText) },
-                        hotReload: false);
-                }
             }
             catch
             {
@@ -118,8 +109,6 @@ public partial class AliasManager : ObservableObject
         }
 
         var keysToRemove = new List<string>();
-        var commandsToDisassociate = new List<string>();
-
         foreach (var kv in aliases)
         {
             // Look for the old aliases for the command, and remove it
@@ -132,32 +121,26 @@ public partial class AliasManager : ObservableObject
             if (newAlias is not null && kv.Value.Alias == newAlias.Alias && kv.Value.CommandId != commandId)
             {
                 keysToRemove.Add(kv.Key);
-                commandsToDisassociate.Add(kv.Value.CommandId);
+
+                // Remove alias from other TopLevelViewModels it may be assigned to
+                var topLevelCommand = _topLevelCommandManager.LookupCommand(kv.Value.CommandId);
+                if (topLevelCommand is not null)
+                {
+                    topLevelCommand.AliasText = string.Empty;
+                }
             }
         }
 
-        // Update settings atomically before touching any ViewModels.
         _settingsService.UpdateSettings(s =>
         {
             var updatedAliases = s.Aliases.RemoveRange(keysToRemove);
 
             if (newAlias is not null)
             {
-                // Use SetItem instead of Add to be resilient to any data inconsistencies
-                // (e.g. orphaned aliases whose key matches the new alias's SearchPrefix).
-                updatedAliases = updatedAliases.SetItem(newAlias.SearchPrefix, newAlias);
+                updatedAliases = updatedAliases.Add(newAlias.SearchPrefix, newAlias);
             }
 
             return s with { Aliases = updatedAliases };
         });
-
-        // After the settings update is complete, notify conflicting ViewModels to clear
-        // their alias text. Doing this after UpdateSettings avoids reentrant calls back
-        // into UpdateAlias (and Save) via the AliasText setter.
-        foreach (var conflictingCommandId in commandsToDisassociate)
-        {
-            var topLevelCommand = _topLevelCommandManager.LookupCommand(conflictingCommandId);
-            topLevelCommand?.ClearAlias();
-        }
     }
 }

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelCommandManager.cs

@@ -47,7 +47,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
     private readonly SupersedingAsyncGate _reloadCommandsGate;
     private CancellationTokenSource _extensionLoadCts = new();
     private CancellationToken _currentExtensionLoadCancellationToken;
-    private int _pendingBackgroundExtensionLoads;
 
     public TopLevelCommandManager(IServiceProvider serviceProvider, ICommandProviderCache commandProviderCache)
     {
@@ -69,8 +68,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
     [ObservableProperty]
     public partial bool IsLoading { get; private set; } = true;
 
-    public bool HasFinishedLoadingCommands => !IsLoading && Volatile.Read(ref _pendingBackgroundExtensionLoads) == 0;
-
     public IEnumerable<CommandProviderWrapper> CommandProviders
     {
         get
@@ -278,7 +275,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
         _extensionLoadCts.Dispose();
         _extensionLoadCts = new();
         _currentExtensionLoadCancellationToken = _extensionLoadCts.Token;
-        Interlocked.Exchange(ref _pendingBackgroundExtensionLoads, 0);
 
         var extensionService = _serviceProvider.GetService<IExtensionService>()!;
         await extensionService.SignalStopExtensionsAsync().ConfigureAwait(false);
@@ -365,7 +361,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
             }
             else if (r.IsTimedOut)
             {
-                TrackBackgroundExtensionLoad(ct);
                 _ = StartExtensionWhenReadyAsync(r.Extension, r.PendingStartTask, r.Stopwatch, ct);
             }
         }
@@ -445,7 +440,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
             // It's weird to repeat the condition here, but it allows the compiler to track nullability of other properties
             if (r.IsTimedOut)
             {
-                TrackBackgroundExtensionLoad(ct);
                 _ = AppendCommandsWhenReadyAsync(r.Wrapper, r.PendingLoadTask, r.Stopwatch, ct);
             }
         }
@@ -505,10 +499,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
         {
             Logger.LogError($"Background start/load of extension {extension.PackageFullName} failed after {sw.ElapsedMilliseconds} ms: {ex}");
         }
-        finally
-        {
-            CompleteBackgroundExtensionLoad(ct);
-        }
     }
 
     private async Task<CommandLoadResult> TryLoadCommandsAsync(CommandProviderWrapper wrapper, CancellationToken ct)
@@ -584,26 +574,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
         {
             Logger.LogError($"Background loading of commands and bands from {wrapper.ExtensionHost?.Extension?.PackageFullName} failed after {sw.ElapsedMilliseconds} ms: {ex}");
         }
-        finally
-        {
-            CompleteBackgroundExtensionLoad(ct);
-        }
-    }
-
-    private void TrackBackgroundExtensionLoad(CancellationToken ct)
-    {
-        if (ct == _currentExtensionLoadCancellationToken)
-        {
-            Interlocked.Increment(ref _pendingBackgroundExtensionLoads);
-        }
-    }
-
-    private void CompleteBackgroundExtensionLoad(CancellationToken ct)
-    {
-        if (ct == _currentExtensionLoadCancellationToken)
-        {
-            Interlocked.Decrement(ref _pendingBackgroundExtensionLoads);
-        }
     }
 
     private void ExtensionService_OnExtensionRemoved(IExtensionService sender, IEnumerable<IExtensionWrapper> extensions)

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelViewModel.cs

@@ -331,22 +331,6 @@ public sealed partial class TopLevelViewModel : ObservableObject, IListItem, IEx
         UpdateTags();
     }
 
-    /// <summary>
-    /// Clears the alias for this command without triggering a recursive save back to settings.
-    /// Called by <see cref="AliasManager"/> after it has already updated settings atomically,
-    /// to avoid reentrant calls into <see cref="AliasManager.UpdateAlias"/> via the <see cref="AliasText"/> setter.
-    /// </summary>
-    internal void ClearAlias()
-    {
-        if (Alias is not null)
-        {
-            Alias = null;
-            OnPropertyChanged(nameof(AliasText));
-            OnPropertyChanged(nameof(IsDirectAlias));
-            UpdateTags();
-        }
-    }
-
     private void FetchAliasFromAliasManager()
     {
         var am = _serviceProvider.GetService<AliasManager>();

src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/AliasManagerTests.cs

@@ -1,148 +0,0 @@
-// 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.Collections.Immutable;
-using System.Reflection;
-using System.Threading.Tasks;
-using CommunityToolkit.Mvvm.Messaging;
-using Microsoft.CmdPal.UI.ViewModels.Services;
-using Microsoft.VisualStudio.TestTools.UnitTesting;
-using Moq;
-
-namespace Microsoft.CmdPal.UI.ViewModels.UnitTests;
-
-[TestClass]
-public class AliasManagerTests
-{
-    [TestMethod]
-    public void CheckAlias_DoesNotRemoveAliasWhileInitialLoadIsStillRunning()
-    {
-        var manager = CreateTopLevelCommandManager();
-        try
-        {
-            SetIsLoading(manager, true);
-            SetPendingBackgroundExtensionLoads(manager, 0);
-
-            var alias = new CommandAlias("test", "test.command", true);
-            var settingsService = CreateSettingsService(alias, out var state);
-            var aliasManager = new AliasManager(manager, settingsService.Object);
-
-            var handled = aliasManager.CheckAlias(alias.SearchPrefix);
-
-            Assert.IsFalse(handled);
-            Assert.IsTrue(state.Current.Aliases.ContainsKey(alias.SearchPrefix));
-            settingsService.Verify(s => s.UpdateSettings(It.IsAny<Func<SettingsModel, SettingsModel>>(), false), Times.Never);
-        }
-        finally
-        {
-            Cleanup(manager);
-        }
-    }
-
-    [TestMethod]
-    public void CheckAlias_DoesNotRemoveAliasWhileBackgroundExtensionLoadsArePending()
-    {
-        var manager = CreateTopLevelCommandManager();
-        try
-        {
-            SetIsLoading(manager, false);
-            SetPendingBackgroundExtensionLoads(manager, 1);
-
-            var alias = new CommandAlias("test", "test.command", true);
-            var settingsService = CreateSettingsService(alias, out var state);
-            var aliasManager = new AliasManager(manager, settingsService.Object);
-
-            var handled = aliasManager.CheckAlias(alias.SearchPrefix);
-
-            Assert.IsFalse(handled);
-            Assert.IsTrue(state.Current.Aliases.ContainsKey(alias.SearchPrefix));
-            settingsService.Verify(s => s.UpdateSettings(It.IsAny<Func<SettingsModel, SettingsModel>>(), false), Times.Never);
-        }
-        finally
-        {
-            Cleanup(manager);
-        }
-    }
-
-    [TestMethod]
-    public void CheckAlias_RemovesAliasAfterAllCommandsFinishLoading()
-    {
-        var manager = CreateTopLevelCommandManager();
-        try
-        {
-            SetIsLoading(manager, false);
-            SetPendingBackgroundExtensionLoads(manager, 0);
-
-            var alias = new CommandAlias("test", "test.command", true);
-            var settingsService = CreateSettingsService(alias, out var state);
-            var aliasManager = new AliasManager(manager, settingsService.Object);
-
-            var handled = aliasManager.CheckAlias(alias.SearchPrefix);
-
-            Assert.IsFalse(handled);
-            Assert.IsFalse(state.Current.Aliases.ContainsKey(alias.SearchPrefix));
-            settingsService.Verify(s => s.UpdateSettings(It.IsAny<Func<SettingsModel, SettingsModel>>(), false), Times.Once);
-        }
-        finally
-        {
-            Cleanup(manager);
-        }
-    }
-
-    private static TopLevelCommandManager CreateTopLevelCommandManager()
-    {
-        var serviceProvider = new Mock<IServiceProvider>();
-        serviceProvider.Setup(p => p.GetService(typeof(TaskScheduler))).Returns(TaskScheduler.Default);
-
-        var commandProviderCache = new Mock<ICommandProviderCache>();
-        return new TopLevelCommandManager(serviceProvider.Object, commandProviderCache.Object);
-    }
-
-    private static Mock<ISettingsService> CreateSettingsService(CommandAlias alias, out SettingsState state)
-    {
-        state = new SettingsState
-        {
-            Current = new SettingsModel
-            {
-                Aliases = ImmutableDictionary<string, CommandAlias>.Empty.Add(alias.SearchPrefix, alias),
-            },
-        };
-
-        var settingsService = new Mock<ISettingsService>();
-        var settingsState = state;
-        settingsService.SetupGet(s => s.Settings).Returns(() => settingsState.Current);
-        settingsService
-            .Setup(s => s.UpdateSettings(It.IsAny<Func<SettingsModel, SettingsModel>>(), It.IsAny<bool>()))
-            .Callback((Func<SettingsModel, SettingsModel> transform, bool _) => settingsState.Current = transform(settingsState.Current));
-
-        return settingsService;
-    }
-
-    private sealed class SettingsState
-    {
-        public SettingsModel Current { get; set; } = new();
-    }
-
-    private static void SetIsLoading(TopLevelCommandManager manager, bool isLoading)
-    {
-        var setter = typeof(TopLevelCommandManager)
-            .GetProperty(nameof(TopLevelCommandManager.IsLoading))!
-            .GetSetMethod(nonPublic: true)!;
-        setter.Invoke(manager, [isLoading]);
-    }
-
-    private static void SetPendingBackgroundExtensionLoads(TopLevelCommandManager manager, int count)
-    {
-        typeof(TopLevelCommandManager)
-            .GetField("_pendingBackgroundExtensionLoads", BindingFlags.Instance | BindingFlags.NonPublic)!
-            .SetValue(manager, count);
-    }
-
-    private static void Cleanup(TopLevelCommandManager manager)
-    {
-        WeakReferenceMessenger.Default.UnregisterAll(manager);
-        manager.Dispose();
-    }
-}

src/settings-ui/Settings.UI/ViewModels/ImageResizerViewModel.cs

@@ -10,6 +10,7 @@ using System.ComponentModel;
 using System.IO;
 using System.Linq;
 using System.Runtime.CompilerServices;
+using System.Timers;
 using global::PowerToys.GPOWrapper;
 using ManagedCommon;
 using Microsoft.PowerToys.Settings.UI.Library;
@@ -18,7 +19,7 @@ using Microsoft.PowerToys.Settings.UI.Library.Interfaces;
 
 namespace Microsoft.PowerToys.Settings.UI.ViewModels;
 
-public partial class ImageResizerViewModel : Observable
+public partial class ImageResizerViewModel : Observable, IDisposable
 {
     private static readonly string DefaultPresetNamePrefix =
         Helpers.ResourceLoaderInstance.ResourceLoader.GetString("ImageResizer_DefaultSize_NewSizePrefix");
@@ -43,6 +44,13 @@ public partial class ImageResizerViewModel : Observable
     /// </summary>
     private readonly ImageSize _customSize;
 
+    // Delay saving of settings in order to avoid calling save multiple times and hitting file in use exception. If there is no other request to save settings in given interval, we proceed to save it; otherwise, we schedule saving it after this interval
+    private const int SaveSettingsDelayInMs = 500;
+
+    private readonly System.Threading.Lock _delayedActionLock = new System.Threading.Lock();
+    private Timer _delayedTimer;
+    private bool _disposed;
+
     private GeneralSettings GeneralSettingsConfig { get; set; }
 
     private readonly SettingsUtils _settingsUtils;
@@ -98,6 +106,11 @@ public partial class ImageResizerViewModel : Observable
         _customSize = Settings.Properties.ImageresizerCustomSize.Value;
 
         _isInitializing = false;
+
+        _delayedTimer = new Timer();
+        _delayedTimer.Interval = SaveSettingsDelayInMs;
+        _delayedTimer.Elapsed += DelayedTimer_Tick;
+        _delayedTimer.AutoReset = false;
     }
 
     private void InitializeEnabledValue()
@@ -337,9 +350,61 @@ public partial class ImageResizerViewModel : Observable
 
     public void SizePropertyChanged(object sender, PropertyChangedEventArgs e)
     {
+        ScheduleSaveImageSizes();
+    }
+
+    private void ScheduleSaveImageSizes()
+    {
+        lock (_delayedActionLock)
+        {
+            if (_disposed)
+            {
+                return;
+            }
+
+            if (_delayedTimer.Enabled)
+            {
+                _delayedTimer.Stop();
+            }
+
+            _delayedTimer.Start();
+        }
+    }
+
+    private void DelayedTimer_Tick(object sender, ElapsedEventArgs e)
+    {
+        lock (_delayedActionLock)
+        {
+            _delayedTimer.Stop();
+        }
+
         SaveImageSizes();
     }
 
+    public void Dispose()
+    {
+        Dispose(true);
+        GC.SuppressFinalize(this);
+    }
+
+    protected virtual void Dispose(bool disposing)
+    {
+        lock (_delayedActionLock)
+        {
+            if (!_disposed)
+            {
+                if (disposing)
+                {
+                    _delayedTimer?.Stop();
+                    _delayedTimer?.Dispose();
+                    _delayedTimer = null;
+                }
+
+                _disposed = true;
+            }
+        }
+    }
+
     private string GenerateNameForNewSize(string namePrefix)
     {
         int newSizeCounter = 0;