diff --git a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSListPageProxy.cs b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSListPageProxy.cs index 967da71d88..0d444f797d 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSListPageProxy.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.JsonRpc/Models/JSListPageProxy.cs @@ -36,6 +36,7 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage private readonly object _stateLock = new(); private readonly JSLazyCache _filters; private readonly JSLazyCache _emptyContent; + private readonly object _getItemsLock = new(); private readonly object _itemCacheLock = new(); private bool? _hasMoreItemsState; private bool _disposed; @@ -108,26 +109,29 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage public IListItem[] GetItems() { - try + lock (_getItemsLock) { - var response = Connection.SendRequestAsync( - "listPage/getItems", - new JsonObject { ["pageId"] = _pageId }, - CancellationToken.None).GetAwaiter().GetResult(); - - if (response.Error != null) + try { - Logger.LogError($"GetItems error for page {_pageId}: {response.Error.Message}"); + var response = Connection.SendRequestAsync( + "listPage/getItems", + new JsonObject { ["pageId"] = _pageId }, + CancellationToken.None).GetAwaiter().GetResult(); + + if (response.Error != null) + { + Logger.LogError($"GetItems error for page {_pageId}: {response.Error.Message}"); + return []; + } + + UpdatePageState(response.Result); + return ParseListItems(response.Result); + } + catch (Exception ex) + { + Logger.LogError($"Failed to get items for page {_pageId}: {ex.Message}"); return []; } - - UpdatePageState(response.Result); - return ParseListItems(response.Result); - } - catch (Exception ex) - { - Logger.LogError($"Failed to get items for page {_pageId}: {ex.Message}"); - return []; } } @@ -451,7 +455,6 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage nextQueue.Enqueue(adapter); } - DisposeAdapters(previousCache); _adapterCache = nextCache; } @@ -462,22 +465,12 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage { lock (_itemCacheLock) { - DisposeAdapters(_adapterCache); + // GetItems hands these adapters to the host, so the cache only owns its + // references. Host-held adapters remain live until their owners release them. _adapterCache = new Dictionary>(StringComparer.Ordinal); } } - private static void DisposeAdapters(Dictionary> cache) - { - foreach (var adapters in cache.Values) - { - while (adapters.TryDequeue(out var adapter)) - { - adapter.Dispose(); - } - } - } - private sealed class PageRegistry { private readonly object _subscribeLock = 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 e757010506..bd7cfe1b18 100644 --- a/src/modules/cmdpal/Tests/Microsoft.CmdPal.JsonRpc.UnitTests/JSAdapterLifecycleTests.cs +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.JsonRpc.UnitTests/JSAdapterLifecycleTests.cs @@ -249,7 +249,7 @@ public partial class JSAdapterTests } [TestMethod] - public void ListPage_RemovedItemDisposesNestedProxies() + public void ListPage_RemovedItemKeepsHostHeldProxyLive() { using var fake = new JSFakeExtension(); var requestCount = 0; @@ -275,14 +275,69 @@ public partial class JSAdapterTests using var page = new JSListPageProxy("page", fake.Connection); var firstItems = page.GetItems(); - _ = firstItems[0].Command; + var command = firstItems[0].Command; Assert.AreEqual(2, JSPropertyChangeRegistry.GetRegistrationCount(fake.Connection, "nested-command")); Assert.AreEqual(0, page.GetItems().Length); + Assert.AreEqual(2, JSPropertyChangeRegistry.GetRegistrationCount(fake.Connection, "nested-command")); + + JSPropertyChangeRegistry.Dispatch( + fake.Connection, + ParseElement(new JsonObject + { + ["commandId"] = "nested-command", + ["properties"] = new JsonObject { ["name"] = "Still live" }, + })); + + Assert.AreEqual("Still live", command?.Name); + + (firstItems[0] as IDisposable)?.Dispose(); Assert.AreEqual(0, JSPropertyChangeRegistry.GetRegistrationCount(fake.Connection, "nested-command")); } + [TestMethod] + public async Task ListPage_ConcurrentGetItemsRequestsAreSerialized() + { + using var fake = new JSFakeExtension(); + var requestCount = 0; + var firstRequestStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var releaseFirstRequest = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + fake.OnRequestAsync("listPage/getItems", async _ => + { + var request = Interlocked.Increment(ref requestCount); + if (request == 1) + { + firstRequestStarted.SetResult(); + await releaseFirstRequest.Task; + } + + return new JsonObject + { + ["items"] = new JsonArray + { + new JsonObject { ["id"] = "row", ["title"] = $"Response {request}" }, + }, + }; + }); + + using var page = new JSListPageProxy("page", fake.Connection); + var firstGetItems = Task.Run(page.GetItems); + await firstRequestStarted.Task.WaitAsync(Timeout); + var secondGetItems = Task.Run(page.GetItems); + + await Task.Delay(100); + Assert.AreEqual(1, Volatile.Read(ref requestCount)); + + releaseFirstRequest.SetResult(); + var firstItems = await firstGetItems.WaitAsync(Timeout); + var secondItems = await secondGetItems.WaitAsync(Timeout); + + Assert.AreEqual("Response 2", secondItems[0].Title); + Assert.AreSame(firstItems[0], secondItems[0]); + Assert.AreEqual("Response 2", firstItems[0].Title); + } + [TestMethod] public void ProviderSettings_ConcurrentReadsReturnOneProxy() { diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.JsonRpc.UnitTests/JSFakeExtension.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.JsonRpc.UnitTests/JSFakeExtension.cs index eb8e0395f0..d912c3c1b1 100644 --- a/src/modules/cmdpal/Tests/Microsoft.CmdPal.JsonRpc.UnitTests/JSFakeExtension.cs +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.JsonRpc.UnitTests/JSFakeExtension.cs @@ -28,8 +28,10 @@ internal sealed class JSFakeExtension : IDisposable private readonly Stream _extensionReads; private readonly Stream _extensionWrites; private readonly ConcurrentDictionary> _handlers = new(StringComparer.Ordinal); + private readonly ConcurrentDictionary>> _asyncHandlers = new(StringComparer.Ordinal); private readonly ConcurrentDictionary _errors = new(StringComparer.Ordinal); private readonly CancellationTokenSource _cts = new(); + private readonly SemaphoreSlim _writeLock = new(1, 1); private readonly Task _pump; private bool _isDisposed; @@ -47,6 +49,8 @@ internal sealed class JSFakeExtension : IDisposable public void OnRequest(string method, Func handler) => _handlers[method] = handler; + public void OnRequestAsync(string method, Func> handler) => _asyncHandlers[method] = handler; + public void OnResult(string method, string resultJson) => _handlers[method] = _ => JsonNode.Parse(resultJson); // Answers a request method with a JSON-RPC error so tests can drive proxy @@ -115,6 +119,12 @@ internal sealed class JSFakeExtension : IDisposable continue; } + if (_asyncHandlers.TryGetValue(method, out var asyncHandler)) + { + _ = RespondAsync(id, asyncHandler(parameters), cancellationToken); + continue; + } + JsonNode? result = null; if (_handlers.TryGetValue(method, out var handler)) { @@ -150,6 +160,20 @@ internal sealed class JSFakeExtension : IDisposable await WriteFramedAsync(message.ToJsonString(), cancellationToken); } + private async Task RespondAsync(int id, Task resultTask, CancellationToken cancellationToken) + { + try + { + await RespondAsync(id, await resultTask, cancellationToken); + } + catch (OperationCanceledException) + { + } + catch (ObjectDisposedException) + { + } + } + private async Task RespondErrorAsync(int id, int code, string message, CancellationToken cancellationToken) { var envelope = new JsonObject @@ -168,14 +192,22 @@ internal sealed class JSFakeExtension : IDisposable private async Task WriteFramedAsync(string json, CancellationToken cancellationToken) { - var body = Encoding.UTF8.GetBytes(json); - var header = Encoding.ASCII.GetBytes($"Content-Length: {body.Length}\r\n\r\n"); - var buffer = new byte[header.Length + body.Length]; - Buffer.BlockCopy(header, 0, buffer, 0, header.Length); - Buffer.BlockCopy(body, 0, buffer, header.Length, body.Length); + await _writeLock.WaitAsync(cancellationToken); + try + { + var body = Encoding.UTF8.GetBytes(json); + var header = Encoding.ASCII.GetBytes($"Content-Length: {body.Length}\r\n\r\n"); + var buffer = new byte[header.Length + body.Length]; + Buffer.BlockCopy(header, 0, buffer, 0, header.Length); + Buffer.BlockCopy(body, 0, buffer, header.Length, body.Length); - await _extensionWrites.WriteAsync(buffer, cancellationToken); - await _extensionWrites.FlushAsync(cancellationToken); + await _extensionWrites.WriteAsync(buffer, cancellationToken); + await _extensionWrites.FlushAsync(cancellationToken); + } + finally + { + _writeLock.Release(); + } } private static async Task ReadFramedAsync(Stream stream, CancellationToken cancellationToken)