From 0358bfbaffcecb1fd858ec189b0f788acae8f587 Mon Sep 17 00:00:00 2001 From: Michael Jolley Date: Thu, 27 Aug 2026 15:39:19 -0500 Subject: [PATCH] Fix JsonRpc proxy registration lifetimes Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 813c51f6-19e2-49ee-bf03-10e3187d3728 --- .../Models/JSCommandItemAdapter.cs | 33 ++- .../Models/JSCommandProviderProxy.cs | 90 +++++-- .../Models/JSCommandSettingsProxy.cs | 16 +- .../Models/JSContentPageProxy.cs | 29 ++- .../Models/JSFallbackCommandItemAdapter.cs | 38 ++- .../Models/JSLazyCache.cs | 87 +++++++ .../Models/JSListItemAdapter.cs | 50 +++- .../Models/JSListPageProxy.cs | 72 +++--- .../Models/JSModelMapper.cs | 41 +++ .../Models/JSPropertyChangeRegistry.cs | 52 +++- .../JSAdapterLifecycleTests.cs | 240 ++++++++++++++++++ 11 files changed, 645 insertions(+), 103 deletions(-) create mode 100644 src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSLazyCache.cs diff --git a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSCommandItemAdapter.cs b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSCommandItemAdapter.cs index 190d1a50a4..e3833e55f2 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSCommandItemAdapter.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSCommandItemAdapter.cs @@ -18,17 +18,23 @@ namespace Microsoft.CmdPal.JsonRpc.Models; /// internal sealed partial class JSCommandItemAdapter : JSObservableProxyBase, ICommandItem { - private Lazy _command; + private readonly JSLazyCache _command; + private readonly JSLazyCache _moreCommands; public JSCommandItemAdapter(JsonElement data, JsonRpcConnection connection) : base(GetNotificationId(data), connection, data) { - _command = CreateCommand(); + _command = new JSLazyCache( + CreateCommand, + JSLazyCache.DisposeValue); + _moreCommands = new JSLazyCache( + () => JSModelMapper.ParseMoreCommands(Data, Connection), + JSModelMapper.DisposeContextItems); } - public ICommand? Command => Volatile.Read(ref _command).Value; + public ICommand? Command => _command.Value; - public IContextItem[] MoreCommands => JSModelMapper.ParseMoreCommands(Data, Connection); + public IContextItem[] MoreCommands => _moreCommands.Value; public IIconInfo Icon => JSModelMapper.TryGetIcon(Data, "icon", out var icon) ? icon @@ -50,16 +56,25 @@ internal sealed partial class JSCommandItemAdapter : JSObservableProxyBase, ICom { if (propertyName == "command") { - Volatile.Write(ref _command, CreateCommand()); - break; + _command.Reset(); + } + else if (propertyName == "moreCommands") + { + _moreCommands.Reset(); } } } - private Lazy CreateCommand() + public override void Dispose() { - return new Lazy( - () => JSCommandFactory.CreateCommandFromJson(JSModelMapper.GetCommandData(Data), Connection)); + _moreCommands.Dispose(); + _command.Dispose(); + base.Dispose(); + } + + private ICommand? CreateCommand() + { + return JSCommandFactory.CreateCommandFromJson(JSModelMapper.GetCommandData(Data), Connection); } private static string GetNotificationId(JsonElement data) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSCommandProviderProxy.cs b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSCommandProviderProxy.cs index 9240149529..4b3cdd19ff 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSCommandProviderProxy.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSCommandProviderProxy.cs @@ -35,6 +35,7 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa // Guards _shownStatusMessages. Host status notifications and Dispose can run // on different threads, so reads, writes, and enumeration share one gate. private readonly object _statusLock = new(); + private readonly object _settingsLock = new(); // Host notifications can arrive after this proxy subscribes but before // InitializeWithHost attaches the host. Buffer them in arrival order so @@ -44,8 +45,9 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa private List? _preInitNotifications = new(); private IExtensionHost? _host; private ICommandSettings? _settingsCache; + private bool _settingsLoading; private bool _settingsQueried; - private bool _isDisposed; + private volatile bool _isDisposed; public JSCommandProviderProxy( JsonRpcConnection connection, @@ -84,39 +86,43 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa { get { - if (_settingsQueried) + lock (_settingsLock) { - return _settingsCache; - } + while (_settingsLoading && !_isDisposed) + { + Monitor.Wait(_settingsLock); + } - _settingsQueried = true; + if (_isDisposed) + { + return null; + } - try - { - var response = _connection.SendRequestAsync( - "provider/getSettings", - null, - CancellationToken.None).GetAwaiter().GetResult(); - - if (response.Error != null || - !response.Result.HasValue || - response.Result.Value.ValueKind != JsonValueKind.Object) + if (_settingsQueried) { return _settingsCache; } - var pageId = JSModelMapper.GetString(response.Result.Value, "id") ?? string.Empty; - if (!string.IsNullOrEmpty(pageId)) - { - _settingsCache = new JSCommandSettingsProxy(pageId, _connection, response.Result.Value.Clone()); - } - } - catch (Exception ex) - { - Logger.LogDebug($"Failed to get settings for {DisplayName}: {ex.Message}"); + _settingsLoading = true; } - return _settingsCache; + var settings = LoadSettings(); + lock (_settingsLock) + { + _settingsLoading = false; + _settingsQueried = true; + if (_isDisposed) + { + (settings as IDisposable)?.Dispose(); + } + else + { + _settingsCache = settings; + } + + Monitor.PulseAll(_settingsLock); + return _settingsCache; + } } } @@ -297,6 +303,12 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa } } + lock (_settingsLock) + { + (_settingsCache as IDisposable)?.Dispose(); + Monitor.PulseAll(_settingsLock); + } + _host = null; } @@ -320,6 +332,34 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa _connection.RegisterNotificationHandler("host/copyText", HandleCopyTextNotification); } + private ICommandSettings? LoadSettings() + { + try + { + var response = _connection.SendRequestAsync( + "provider/getSettings", + null, + CancellationToken.None).GetAwaiter().GetResult(); + + if (response.Error != null || + !response.Result.HasValue || + response.Result.Value.ValueKind != JsonValueKind.Object) + { + return null; + } + + var pageId = JSModelMapper.GetString(response.Result.Value, "id") ?? string.Empty; + return string.IsNullOrEmpty(pageId) + ? null + : new JSCommandSettingsProxy(pageId, _connection, response.Result.Value.Clone()); + } + catch (Exception ex) + { + Logger.LogDebug($"Failed to get settings for {DisplayName}: {ex.Message}"); + return null; + } + } + // Buffers a host notification until InitializeWithHost attaches the host. // The params element is cloned because the connection may recycle the source document. private bool TryBufferUntilHostAttached(string method, JsonElement paramsElement) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSCommandSettingsProxy.cs b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSCommandSettingsProxy.cs index 5a23e48fa9..c4d6b8a9c2 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSCommandSettingsProxy.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSCommandSettingsProxy.cs @@ -13,18 +13,18 @@ namespace Microsoft.CmdPal.JsonRpc.Models; /// The full settings page payload is kept intact, including title, name, icon, /// details, and commands. /// -internal sealed partial class JSCommandSettingsProxy : ICommandSettings +internal sealed partial class JSCommandSettingsProxy : ICommandSettings, IDisposable { - private readonly string _settingsPageId; - private readonly JsonRpcConnection _connection; - private readonly JsonElement _settingsPageData; + private readonly JSLazyCache _settingsPage; public JSCommandSettingsProxy(string settingsPageId, JsonRpcConnection connection, JsonElement settingsPageData = default) { - _settingsPageId = settingsPageId; - _connection = connection; - _settingsPageData = settingsPageData; + _settingsPage = new JSLazyCache( + () => new JSContentPageProxy(settingsPageId, connection, settingsPageData), + JSLazyCache.DisposeValue); } - public IContentPage SettingsPage => new JSContentPageProxy(_settingsPageId, _connection, _settingsPageData); + public IContentPage SettingsPage => _settingsPage.Value; + + public void Dispose() => _settingsPage.Dispose(); } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSContentPageProxy.cs b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSContentPageProxy.cs index 95dcff910c..845125f617 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSContentPageProxy.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSContentPageProxy.cs @@ -29,11 +29,19 @@ internal sealed partial class JSContentPageProxy : JSObservableProxyBase, IConte private readonly string _pageId; private readonly PageRegistry _registry; + private readonly JSLazyCache _details; + private readonly JSLazyCache _commands; public JSContentPageProxy(string pageId, JsonRpcConnection connection, JsonElement pageData = default) : base(pageId, connection, pageData) { _pageId = pageId ?? throw new ArgumentNullException(nameof(pageId)); + _details = new JSLazyCache( + () => JSModelMapper.ParseDetails(Data, Connection), + JSModelMapper.DisposeDetails); + _commands = new JSLazyCache( + () => JSModelMapper.ParseContextItems(Data, "commands", Connection), + JSModelMapper.DisposeContextItems); _registry = Registries.GetValue(Connection, static _ => new PageRegistry()); _registry.EnsureSubscribed(Connection); @@ -59,9 +67,9 @@ internal sealed partial class JSContentPageProxy : JSObservableProxyBase, IConte public OptionalColor AccentColor => JSModelMapper.ParseColor(Data, "accentColor"); - public IDetails? Details => JSModelMapper.ParseDetails(Data, Connection); + public IDetails? Details => _details.Value; - public IContextItem[] Commands => JSModelMapper.ParseContextItems(Data, "commands", Connection); + public IContextItem[] Commands => _commands.Value; public IContent[] GetContent() { @@ -89,6 +97,8 @@ internal sealed partial class JSContentPageProxy : JSObservableProxyBase, IConte public override void Dispose() { + _details.Dispose(); + _commands.Dispose(); if (_registry.Pages.TryGetValue(_pageId, out var pages)) { lock (pages) @@ -111,6 +121,21 @@ internal sealed partial class JSContentPageProxy : JSObservableProxyBase, IConte _ => false, }; + protected override void OnPropertyChangesApplied(IReadOnlyList propertyNames) + { + foreach (var propertyName in propertyNames) + { + if (propertyName == "details") + { + _details.Reset(); + } + else if (propertyName == "commands") + { + _commands.Reset(); + } + } + } + private static JsonElement? UnwrapContent(JsonElement? result) { if (!result.HasValue) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSFallbackCommandItemAdapter.cs b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSFallbackCommandItemAdapter.cs index af7b12e94a..b5952c6ff6 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSFallbackCommandItemAdapter.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSFallbackCommandItemAdapter.cs @@ -21,18 +21,24 @@ namespace Microsoft.CmdPal.JsonRpc.Models; internal sealed partial class JSFallbackCommandItemAdapter : JSObservableProxyBase, IFallbackCommandItem2 { private readonly object _commandStateLock = new(); - private Lazy _command; + private readonly JSLazyCache _command; + private readonly JSLazyCache _moreCommands; private IFallbackHandler? _fallbackHandler; public JSFallbackCommandItemAdapter(JsonElement data, JsonRpcConnection connection) : base(GetNotificationId(data), connection, data) { - _command = CreateCommand(); + _command = new JSLazyCache( + CreateCommand, + JSLazyCache.DisposeValue); + _moreCommands = new JSLazyCache( + () => JSModelMapper.ParseMoreCommands(Data, Connection), + JSModelMapper.DisposeContextItems); } - public ICommand? Command => Volatile.Read(ref _command).Value; + public ICommand? Command => _command.Value; - public IContextItem[] MoreCommands => JSModelMapper.ParseMoreCommands(Data, Connection); + public IContextItem[] MoreCommands => _moreCommands.Value; public IIconInfo Icon => JSModelMapper.TryGetIcon(Data, "icon", out var icon) ? icon @@ -72,19 +78,31 @@ internal sealed partial class JSFallbackCommandItemAdapter : JSObservableProxyBa { lock (_commandStateLock) { - Volatile.Write(ref _command, CreateCommand()); + _command.Reset(); _fallbackHandler = null; } - - break; + } + else if (propertyName == "moreCommands") + { + _moreCommands.Reset(); } } } - private Lazy CreateCommand() + public override void Dispose() { - return new Lazy( - () => JSCommandFactory.CreateCommandFromJson(JSModelMapper.GetCommandData(Data), Connection)); + _moreCommands.Dispose(); + lock (_commandStateLock) + { + _command.Dispose(); + } + + base.Dispose(); + } + + private ICommand? CreateCommand() + { + return JSCommandFactory.CreateCommandFromJson(JSModelMapper.GetCommandData(Data), Connection); } private static string GetNotificationId(JsonElement data) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSLazyCache.cs b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSLazyCache.cs new file mode 100644 index 0000000000..b5c6d983fe --- /dev/null +++ b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSLazyCache.cs @@ -0,0 +1,87 @@ +// 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; + +namespace Microsoft.CmdPal.JsonRpc.Models; + +internal sealed partial class JSLazyCache : IDisposable +{ + private readonly object _lock = new(); + private readonly Func _factory; + private readonly Action? _dispose; + private T _value = default!; + private bool _hasValue; + private bool _disposed; + + internal JSLazyCache(Func factory, Action? dispose = null) + { + _factory = factory; + _dispose = dispose; + } + + internal T Value + { + get + { + lock (_lock) + { + ObjectDisposedException.ThrowIf(_disposed, this); + + if (!_hasValue) + { + _value = _factory(); + _hasValue = true; + } + + return _value; + } + } + } + + public void Dispose() + { + lock (_lock) + { + if (_disposed) + { + return; + } + + _disposed = true; + DisposeCreatedValue(); + } + } + + internal void Reset() + { + lock (_lock) + { + if (_disposed) + { + return; + } + + DisposeCreatedValue(); + _value = default!; + _hasValue = false; + } + } + + internal static void DisposeValue(T value) + { + if (value is IDisposable disposable) + { + disposable.Dispose(); + } + } + + private void DisposeCreatedValue() + { + if (_hasValue) + { + _dispose?.Invoke(_value); + } + } +} diff --git a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSListItemAdapter.cs b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSListItemAdapter.cs index 896a154326..9650c061d1 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSListItemAdapter.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSListItemAdapter.cs @@ -19,17 +19,27 @@ namespace Microsoft.CmdPal.JsonRpc.Models; /// internal sealed partial class JSListItemAdapter : JSObservableProxyBase, IListItem { - private Lazy _command; + private readonly JSLazyCache _command; + private readonly JSLazyCache _moreCommands; + private readonly JSLazyCache _details; public JSListItemAdapter(JsonElement data, JsonRpcConnection connection) : base(GetNotificationId(data), connection, data) { - _command = CreateCommand(); + _command = new JSLazyCache( + CreateCommand, + JSLazyCache.DisposeValue); + _moreCommands = new JSLazyCache( + () => JSModelMapper.ParseMoreCommands(Data, Connection), + JSModelMapper.DisposeContextItems); + _details = new JSLazyCache( + () => JSModelMapper.ParseDetails(Data, Connection), + JSModelMapper.DisposeDetails); } - public ICommand? Command => Volatile.Read(ref _command).Value; + public ICommand? Command => _command.Value; - public IContextItem[] MoreCommands => JSModelMapper.ParseMoreCommands(Data, Connection); + public IContextItem[] MoreCommands => _moreCommands.Value; public IIconInfo Icon => JSModelMapper.TryGetIcon(Data, "icon", out var icon) ? icon @@ -41,7 +51,7 @@ internal sealed partial class JSListItemAdapter : JSObservableProxyBase, IListIt public ITag[] Tags => JSModelMapper.ParseTags(Data); - public IDetails? Details => JSModelMapper.ParseDetails(Data, Connection); + public IDetails? Details => _details.Value; public string Section => JSModelMapper.GetString(Data, "section") ?? string.Empty; @@ -60,20 +70,32 @@ internal sealed partial class JSListItemAdapter : JSObservableProxyBase, IListIt { if (propertyName == "command") { - Volatile.Write(ref _command, CreateCommand()); - break; + _command.Reset(); + } + else if (propertyName == "moreCommands") + { + _moreCommands.Reset(); + } + else if (propertyName == "details") + { + _details.Reset(); } } } - private Lazy CreateCommand() + public override void Dispose() { - return new Lazy(() => - { - return JSModelMapper.TryGetCommandData(Data, out var commandData) - ? JSCommandFactory.CreateCommandFromJson(commandData, Connection) - : null; - }); + _moreCommands.Dispose(); + _details.Dispose(); + _command.Dispose(); + base.Dispose(); + } + + private ICommand? CreateCommand() + { + return JSModelMapper.TryGetCommandData(Data, out var commandData) + ? JSCommandFactory.CreateCommandFromJson(commandData, Connection) + : null; } private static string GetNotificationId(JsonElement data) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSListPageProxy.cs b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSListPageProxy.cs index 285f5e0e69..2a42d132a3 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSListPageProxy.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSListPageProxy.cs @@ -35,6 +35,8 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage private readonly string _pageId; private readonly PageRegistry _registry; private readonly object _stateLock = new(); + private readonly JSLazyCache _filters; + private readonly JSLazyCache _emptyContent; private bool? _hasMoreItemsState; private bool _disposed; @@ -42,6 +44,10 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage : base(pageId, connection, pageData) { _pageId = pageId ?? throw new ArgumentNullException(nameof(pageId)); + _filters = new JSLazyCache(CreateFilters); + _emptyContent = new JSLazyCache( + CreateEmptyContent, + JSLazyCache.DisposeValue); // Get the retained registry before subscribing. ConditionalWeakTable may run // the factory on a thread that loses the race and discards its result. If the @@ -77,19 +83,7 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage public bool ShowDetails => JSModelMapper.GetBool(Data, "showDetails", false); - public IFilters? Filters - { - get - { - if (JSModelMapper.TryGetProperty(Data, "filters", out var filtersProp) && - filtersProp.ValueKind == JsonValueKind.Object) - { - return new JSFiltersAdapter(filtersProp, Connection, _pageId); - } - - return null; - } - } + public IFilters? Filters => _filters.Value; public IGridProperties? GridProperties => JSModelMapper.ParseGridProperties(Data); @@ -107,19 +101,7 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage } } - public ICommandItem? EmptyContent - { - get - { - if (JSModelMapper.TryGetProperty(Data, "emptyContent", out var emptyProp) && - emptyProp.ValueKind == JsonValueKind.Object) - { - return new JSCommandItemAdapter(emptyProp, Connection); - } - - return null; - } - } + public ICommandItem? EmptyContent => _emptyContent.Value; public IListItem[] GetItems() { @@ -309,6 +291,8 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage _disposed = true; + _filters.Dispose(); + _emptyContent.Dispose(); base.Dispose(); if (_registry.Pages.TryGetValue(_pageId, out var list)) @@ -395,18 +379,44 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage protected override void OnPropertyChangesApplied(IReadOnlyList propertyNames) { - if (!propertyNames.Contains("hasMoreItems")) + foreach (var propertyName in propertyNames) { - return; + if (propertyName == "filters") + { + _filters.Reset(); + } + else if (propertyName == "emptyContent") + { + _emptyContent.Reset(); + } } - var value = JSModelMapper.GetBool(Data, "hasMoreItems", false); - lock (_stateLock) + if (propertyNames.Contains("hasMoreItems")) { - _hasMoreItemsState = value; + var value = JSModelMapper.GetBool(Data, "hasMoreItems", false); + lock (_stateLock) + { + _hasMoreItemsState = value; + } } } + private IFilters? CreateFilters() + { + return JSModelMapper.TryGetProperty(Data, "filters", out var filtersProp) && + filtersProp.ValueKind == JsonValueKind.Object + ? new JSFiltersAdapter(filtersProp, Connection, _pageId) + : null; + } + + private ICommandItem? CreateEmptyContent() + { + return JSModelMapper.TryGetProperty(Data, "emptyContent", out var emptyProp) && + emptyProp.ValueKind == JsonValueKind.Object + ? new JSCommandItemAdapter(emptyProp, Connection) + : null; + } + private IListItem[] ParseListItems(JsonElement? result) { if (!result.HasValue) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSModelMapper.cs b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSModelMapper.cs index 30147022de..ba791bc546 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSModelMapper.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSModelMapper.cs @@ -284,6 +284,47 @@ internal static class JSModelMapper return items.ToArray(); } + internal static void DisposeContextItems(IEnumerable items) + { + foreach (var item in items) + { + if (item is not ICommandContextItem commandItem) + { + continue; + } + + DisposeContextItems(commandItem.MoreCommands); + if (commandItem.Command is IDisposable disposable) + { + disposable.Dispose(); + } + } + } + + internal static void DisposeDetails(IDetails? details) + { + if (details is null) + { + return; + } + + foreach (var element in details.Metadata) + { + if (element.Data is not IDetailsCommands commands) + { + continue; + } + + foreach (var command in commands.Commands ?? []) + { + if (command is IDisposable disposable) + { + disposable.Dispose(); + } + } + } + } + internal static ICommandContextItem ParseContextItem(JsonElement element, JsonRpcConnection connection) { var command = JSCommandFactory.CreateCommandFromJson(GetCommandData(element), connection); diff --git a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSPropertyChangeRegistry.cs b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSPropertyChangeRegistry.cs index e322ad781f..ab78985aca 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSPropertyChangeRegistry.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSPropertyChangeRegistry.cs @@ -18,10 +18,26 @@ internal static class JSPropertyChangeRegistry internal static void Register(JsonRpcConnection connection, string commandId, IJSPropertyChangeTarget target) { var registry = Registries.GetValue(connection, static _ => new Registry()); - var targets = registry.Targets.GetOrAdd(commandId, static _ => []); - lock (targets) + while (true) { - targets.Add(new WeakReference(target)); + var targets = registry.Targets.GetOrAdd(commandId, static _ => []); + lock (targets) + { + if (!registry.Targets.TryGetValue(commandId, out var currentTargets) || + !ReferenceEquals(targets, currentTargets)) + { + continue; + } + + targets.RemoveAll(reference => !reference.TryGetTarget(out _)); + if (!targets.Exists(reference => + reference.TryGetTarget(out var current) && ReferenceEquals(current, target))) + { + targets.Add(new WeakReference(target)); + } + + return; + } } } @@ -39,7 +55,7 @@ internal static class JSPropertyChangeRegistry !reference.TryGetTarget(out var current) || ReferenceEquals(current, target)); if (targets.Count == 0) { - registry.Targets.TryRemove(commandId, out _); + RemoveTargets(registry, commandId, targets); } } } @@ -73,6 +89,11 @@ internal static class JSPropertyChangeRegistry liveTargets.Add(target); } } + + if (targets.Count == 0) + { + RemoveTargets(registry, commandId, targets); + } } foreach (var target in liveTargets) @@ -81,6 +102,29 @@ internal static class JSPropertyChangeRegistry } } + internal static int GetRegistrationCount(JsonRpcConnection connection, string commandId) + { + if (!Registries.TryGetValue(connection, out var registry) || + !registry.Targets.TryGetValue(commandId, out var targets)) + { + return 0; + } + + lock (targets) + { + return targets.Count; + } + } + + private static void RemoveTargets( + Registry registry, + string commandId, + List> targets) + { + ((ICollection>>>)registry.Targets) + .Remove(new KeyValuePair>>(commandId, targets)); + } + private sealed class Registry { internal ConcurrentDictionary>> Targets { get; } = new(); diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.JsonRpc.UnitTests/JSAdapterLifecycleTests.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.JsonRpc.UnitTests/JSAdapterLifecycleTests.cs index 5ea3c09b0c..87c3e3b4b9 100644 --- a/src/modules/cmdpal/Tests/Microsoft.CmdPal.JsonRpc.UnitTests/JSAdapterLifecycleTests.cs +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.JsonRpc.UnitTests/JSAdapterLifecycleTests.cs @@ -3,10 +3,13 @@ // See the LICENSE file in the project root for more information. using System; +using System.Runtime.CompilerServices; +using System.Text.Json; using System.Text.Json.Nodes; using System.Threading; using System.Threading.Tasks; using Microsoft.CmdPal.JsonRpc.Models; +using Microsoft.CommandPalette.Extensions; using Microsoft.CommandPalette.Extensions.Toolkit; using Microsoft.VisualStudio.TestTools.UnitTesting; @@ -14,6 +17,204 @@ namespace Microsoft.CmdPal.JsonRpc.UnitTests; public partial class JSAdapterTests { + [TestMethod] + public void PropertyChangeRegistry_PrunesDeadTargetsAndDeduplicatesLiveTargets() + { + using var fake = new JSFakeExtension(); + const string commandId = "shared-command"; + var deadTarget = RegisterTemporaryTarget(fake.Connection, commandId); + + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + + Assert.IsFalse(deadTarget.TryGetTarget(out _)); + Assert.AreEqual(1, JSPropertyChangeRegistry.GetRegistrationCount(fake.Connection, commandId)); + + var liveTarget = new RecordingPropertyChangeTarget(); + Parallel.For( + 0, + 64, + _ => JSPropertyChangeRegistry.Register(fake.Connection, commandId, liveTarget)); + + Assert.AreEqual(1, JSPropertyChangeRegistry.GetRegistrationCount(fake.Connection, commandId)); + + JSPropertyChangeRegistry.Dispatch( + fake.Connection, + ParseElement(new JsonObject + { + ["commandId"] = commandId, + ["properties"] = new JsonObject { ["title"] = "updated" }, + })); + + Assert.AreEqual(1, liveTarget.ApplyCount); + JSPropertyChangeRegistry.Unregister(fake.Connection, commandId, liveTarget); + } + + [TestMethod] + public void NestedProxyGetters_CacheIdentityAndRegistration() + { + using var fake = new JSFakeExtension(); + using var settings = new JSCommandSettingsProxy("settings", fake.Connection); + using var listPage = new JSListPageProxy( + "list-page", + fake.Connection, + ParseElement(new JsonObject + { + ["id"] = "list-page", + ["filters"] = new JsonObject { ["filters"] = new JsonArray() }, + ["emptyContent"] = CommandItem("empty-command"), + })); + using var contentPage = new JSContentPageProxy( + "content-page", + fake.Connection, + ParseElement(new JsonObject + { + ["id"] = "content-page", + ["details"] = new JsonObject { ["title"] = "Details" }, + ["commands"] = new JsonArray(ContextItem("content-command")), + })); + using var commandItem = new JSCommandItemAdapter( + ParseElement(new JsonObject + { + ["id"] = "command-item", + ["title"] = "Command", + ["moreCommands"] = new JsonArray(ContextItem("command-more")), + }), + fake.Connection); + using var listItem = new JSListItemAdapter( + ParseElement(new JsonObject + { + ["id"] = "list-item", + ["title"] = "List", + ["details"] = new JsonObject { ["title"] = "Details" }, + ["moreCommands"] = new JsonArray(ContextItem("list-more")), + }), + fake.Connection); + using var fallbackItem = new JSFallbackCommandItemAdapter( + ParseElement(new JsonObject + { + ["id"] = "fallback-item", + ["title"] = "Fallback", + ["command"] = Command("fallback-command"), + ["moreCommands"] = new JsonArray(ContextItem("fallback-more")), + }), + fake.Connection); + var concurrentSettingsPages = new IContentPage[64]; + Parallel.For(0, concurrentSettingsPages.Length, i => concurrentSettingsPages[i] = settings.SettingsPage); + + foreach (var settingsPage in concurrentSettingsPages) + { + Assert.AreSame(settings.SettingsPage, settingsPage); + } + + Assert.AreSame(listPage.Filters, listPage.Filters); + Assert.AreSame(listPage.EmptyContent, listPage.EmptyContent); + Assert.AreSame(contentPage.Details, contentPage.Details); + Assert.AreSame(contentPage.Commands, contentPage.Commands); + Assert.AreSame(commandItem.MoreCommands, commandItem.MoreCommands); + Assert.AreSame(listItem.MoreCommands, listItem.MoreCommands); + Assert.AreSame(listItem.Details, listItem.Details); + Assert.AreSame(fallbackItem.MoreCommands, fallbackItem.MoreCommands); + + Assert.AreEqual(1, JSPropertyChangeRegistry.GetRegistrationCount(fake.Connection, "empty-command")); + Assert.AreEqual(1, JSPropertyChangeRegistry.GetRegistrationCount(fake.Connection, "content-command")); + Assert.AreEqual(1, JSPropertyChangeRegistry.GetRegistrationCount(fake.Connection, "command-more")); + Assert.AreEqual(1, JSPropertyChangeRegistry.GetRegistrationCount(fake.Connection, "list-more")); + Assert.AreEqual(1, JSPropertyChangeRegistry.GetRegistrationCount(fake.Connection, "fallback-more")); + } + + [TestMethod] + public void NestedProxyGetters_InvalidateOnlyTheChangedProperty() + { + using var fake = new JSFakeExtension(); + using var item = new JSListItemAdapter( + ParseElement(new JsonObject + { + ["id"] = "list-item", + ["title"] = "Before", + ["details"] = new JsonObject { ["title"] = "Old details" }, + ["moreCommands"] = new JsonArray(ContextItem("old-more")), + }), + fake.Connection); + var originalDetails = item.Details; + var originalMoreCommands = item.MoreCommands; + + JSPropertyChangeRegistry.Dispatch( + fake.Connection, + ParseElement(new JsonObject + { + ["commandId"] = "list-item", + ["properties"] = new JsonObject { ["title"] = "After" }, + })); + + Assert.AreEqual("After", item.Title); + Assert.AreSame(originalDetails, item.Details); + Assert.AreSame(originalMoreCommands, item.MoreCommands); + + JSPropertyChangeRegistry.Dispatch( + fake.Connection, + ParseElement(new JsonObject + { + ["commandId"] = "list-item", + ["properties"] = new JsonObject + { + ["details"] = new JsonObject { ["title"] = "New details" }, + ["moreCommands"] = new JsonArray(ContextItem("new-more")), + }, + })); + + Assert.AreEqual("New details", item.Details?.Title); + Assert.AreNotSame(originalDetails, item.Details); + Assert.AreNotSame(originalMoreCommands, item.MoreCommands); + Assert.AreEqual(0, JSPropertyChangeRegistry.GetRegistrationCount(fake.Connection, "old-more")); + Assert.AreEqual(1, JSPropertyChangeRegistry.GetRegistrationCount(fake.Connection, "new-more")); + } + + [TestMethod] + public void ProviderSettings_ConcurrentReadsReturnOneProxy() + { + using var fake = new JSFakeExtension(); + var requestCount = 0; + fake.OnRequest("provider/getSettings", _ => + { + Interlocked.Increment(ref requestCount); + return new JsonObject { ["id"] = "settings-page" }; + }); + using var provider = CreateProvider(fake); + var settings = new ICommandSettings?[64]; + + Parallel.For(0, settings.Length, i => settings[i] = provider.Settings); + + Assert.AreEqual(1, requestCount); + foreach (var current in settings) + { + Assert.AreSame(settings[0], current); + } + } + + [TestMethod] + public async Task ProviderSettings_DisposeDuringRequestDoesNotPublishProxy() + { + using var fake = new JSFakeExtension(); + using var requestStarted = new ManualResetEventSlim(); + using var releaseRequest = new ManualResetEventSlim(); + fake.OnRequest("provider/getSettings", _ => + { + requestStarted.Set(); + releaseRequest.Wait(Timeout); + return new JsonObject { ["id"] = "settings-page" }; + }); + using var provider = CreateProvider(fake); + + var settings = Task.Run(() => provider.Settings); + Assert.IsTrue(requestStarted.Wait(Timeout)); + provider.Dispose(); + releaseRequest.Set(); + + Assert.IsNull(await settings.WaitAsync(Timeout)); + } + // LoadMore folds the loaded page into pagination state and raises // ItemsChanged so the host asks GetItems again and sees the appended items. // It stops once the extension reports the final page. @@ -216,4 +417,43 @@ public partial class JSAdapterTests Assert.IsTrue(host.HiddenCount >= 1); Assert.IsTrue(host.HiddenCount <= host.ShownCount); } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static WeakReference RegisterTemporaryTarget( + JsonRpcConnection connection, + string commandId) + { + var target = new RecordingPropertyChangeTarget(); + JSPropertyChangeRegistry.Register(connection, commandId, target); + return new WeakReference(target); + } + + private static JsonObject Command(string id) => new() + { + ["id"] = id, + ["name"] = id, + }; + + private static JsonObject CommandItem(string id) => new() + { + ["id"] = id, + ["title"] = id, + ["command"] = Command(id), + }; + + private static JsonObject ContextItem(string id) => new() + { + ["title"] = id, + ["command"] = Command(id), + }; + + private sealed class RecordingPropertyChangeTarget : IJSPropertyChangeTarget + { + public int ApplyCount { get; private set; } + + public void ApplyPropertyChanges(JsonElement properties) + { + ApplyCount++; + } + } }