diff --git a/src/componentsBase/BaseRendererControl.cs b/src/componentsBase/BaseRendererControl.cs index 3d4fdad2..e7ad8577 100644 --- a/src/componentsBase/BaseRendererControl.cs +++ b/src/componentsBase/BaseRendererControl.cs @@ -1126,7 +1126,11 @@ private IgbJsonContext SerializerContext var ret = await SendMessageImmediate(m); TaskCompletionSource tcs = new TaskCompletionSource(); - _methodTasks.Add(invokeId, tcs); + // A return can arrive on the dispatcher while a call made off it is still getting here. + lock (_semLock) + { + _methodTasks[invokeId] = tcs; + } if (ret is JsonElement && ((JsonElement)ret).ValueKind == JsonValueKind.String) { @@ -1139,20 +1143,29 @@ private IgbJsonContext SerializerContext ((JsonElement)retDict["retType"]).GetString() == "promise") { // did we already get a value returned for this method invoke before we could start up a task? - if (_methodReturns.ContainsKey(invokeId)) + object? early; + bool arrivedEarly; + lock (_semLock) { - tcs.SetResult(_methodReturns[invokeId]); + arrivedEarly = _methodReturns.TryGetValue(invokeId, out early); + } + if (arrivedEarly) + { + tcs.TrySetResult(early); } } else { - tcs.SetResult(ret); + tcs.TrySetResult(ret); } } var result = await tcs.Task; - _methodTasks.Remove(invokeId); - _methodReturns.Remove(invokeId); + lock (_semLock) + { + _methodTasks.Remove(invokeId); + _methodReturns.Remove(invokeId); + } return result; } @@ -1563,15 +1576,14 @@ private void SendMessage(RendererMessage m) private async Task SendMessageImmediate(RendererMessage m) { - if (disposedValue) - { - return null; - } - - // The send must start under this lock. + // The send must start under this lock, which is also where disposal closes it. Task sent; lock (_messageQueueLock) { + if (disposedValue) + { + return null; + } Update(); sent = SendJsonImmediate(m); } @@ -1580,12 +1592,12 @@ private void SendMessage(RendererMessage m) private object? SendMessageSyncImmediate(RendererMessage m) { - if (disposedValue) - { - return null; - } lock (_messageQueueLock) { + if (disposedValue) + { + return null; + } UpdateSync(); return SendJsonImmediateSync(m); } @@ -1595,6 +1607,11 @@ private void Enqueue(RendererMessage m) { lock (_messageQueueLock) { + // Also checked here so disposal cannot land between a caller's check and its enqueue. + if (disposedValue) + { + return; + } _messageQueue.AddLast(m); } } @@ -1625,7 +1642,7 @@ private void Update() { this._updateQueued = false; - if (!_ready) + if (!_ready || disposedValue) { return; } @@ -1646,7 +1663,7 @@ private void UpdateSync() { this._updateQueued = false; - if (!_ready) + if (!_ready || disposedValue) { return; } @@ -2139,14 +2156,16 @@ internal void OnInvokeReturn(long invokeId, Object returnValue) } InvokeAsync(() => { - if (_methodTasks.ContainsKey(invokeId)) - { - _methodTasks[invokeId].SetResult(result); - } - else + TaskCompletionSource? waiting; + lock (_semLock) { - _methodReturns.Add(invokeId, result); + if (!_methodTasks.TryGetValue(invokeId, out waiting)) + { + _methodReturns[invokeId] = result; + } } + // Completed outside the lock: the continuation runs inline on this thread. + waiting?.TrySetResult(result); }); } @@ -3265,12 +3284,16 @@ internal void OnRaiseEvent(string name, string propertyName, string args) /// public virtual async ValueTask DisposeAsync() { - if (disposedValue) + // Published under the queue's lock, so nothing can enqueue once teardown has begun. + lock (_messageQueueLock) { - return; + if (disposedValue) + { + return; + } + disposedValue = true; } - disposedValue = true; _shouldReevaluateRuntime = true; try diff --git a/tests/IgniteUI.Blazor.Tests/BaseRendererControlDisposalTests.cs b/tests/IgniteUI.Blazor.Tests/BaseRendererControlDisposalTests.cs index c8c5dab8..cf22eeb9 100644 --- a/tests/IgniteUI.Blazor.Tests/BaseRendererControlDisposalTests.cs +++ b/tests/IgniteUI.Blazor.Tests/BaseRendererControlDisposalTests.cs @@ -1,10 +1,24 @@ +using System.Collections.ObjectModel; +using System.Text.Json; using Bunit; using IgniteUI.Blazor.Controls; using Microsoft.JSInterop; -using System.Text.Json; namespace IgniteUI.Blazor.Tests; +/// +/// Runs alone: one of these holds the thread pool, which every component's flush needs, so it +/// must not run alongside tests that are waiting for one. +/// +// TODO: on xUnit v3 4.0+ this can narrow to [Fact(DisableParallelism = true)] on the one test +// that holds the pool - but only under ParallelMode.All, since the marker is ignored in the +// default collections mode. https://xunit.net/docs/running-tests-in-parallel +[CollectionDefinition(Name, DisableParallelization = true)] +public sealed class DisposalCollection +{ + public const string Name = "renderer disposal"; +} + /// /// Tests around — the async disposal /// path must be resilient to JS interop failures, since disposal typically runs @@ -14,6 +28,7 @@ namespace IgniteUI.Blazor.Tests; /// (https://learn.microsoft.com/aspnet/core/blazor/components/component-disposal): /// when both are implemented the framework only invokes the async overload. /// +[Collection(DisposalCollection.Name)] public class BaseRendererControlDisposalTests : BlazorComponentTestBase { [Fact] @@ -131,4 +146,85 @@ private void AssertCleanupSentOnce(int invocationCount) using var message = JsonDocument.Parse((string)cleanup.Arguments[1]!); Assert.Equal("cleanup", message.RootElement.GetProperty("type").GetString()); } + + private sealed record Row(string Name); + + private IReadOnlyList MessageTypesFor(string containerId) + { + var types = new List(); + foreach (var invocation in JSInterop.Invocations.Where(v => + v.Identifier == "igSendMessage" && v.Arguments.Count > 1 && v.Arguments[0] as string == containerId)) + { + using var message = JsonDocument.Parse((string)invocation.Arguments[1]!); + types.Add(message.RootElement.GetProperty("type").GetString()); + } + + return types; + } + + [Fact] + public async Task DisposeAsync_StopsAFlushScheduledBeforeIt() + { + Interop.PrimeReady(); + var data = new ObservableCollection(); + var cut = Render>(ps => ps.Add(c => c.Data, data)); + var id = Interop.ContainerIdOf(cut); + + // The flush is posted through a thread-pool work item, so holding the pool leaves one + // scheduled but unable to run - the state teardown has to refuse rather than let land. + ThreadPool.GetMinThreads(out var workers, out var completionPorts); + ThreadPool.SetMinThreads(1, completionPorts); + using var release = new ManualResetEventSlim(false); + for (var i = 0; i < 128; i++) + { + ThreadPool.UnsafeQueueUserWorkItem(_ => release.Wait(10000), null); + } + + data.Add(new Row("queued")); // enqueued; flush scheduled, cannot run + var beforeDisposal = MessageTypesFor(id).Count; + + await ((IAsyncDisposable)cut.Instance).DisposeAsync(); + + release.Set(); + ThreadPool.SetMinThreads(workers, completionPorts); + await Task.Delay(250); + + var afterDisposal = MessageTypesFor(id).Skip(beforeDisposal).ToList(); + Assert.Collection(afterDisposal, type => Assert.Equal("cleanup", type)); + } + + [Fact] + public async Task DisposeAsync_SendsNothingAfterCleanup() + { + Interop.PrimeReady(); + var data = new ObservableCollection(); + var cut = Render>(ps => ps.Add(c => c.Data, data)); + var id = Interop.ContainerIdOf(cut); + + // A producer that does not know the component is going away, so it keeps reaching the + // queue across teardown - the case the disposed check has to close. + using var stop = new CancellationTokenSource(); + var producer = Task.Run(() => + { + while (!stop.IsCancellationRequested) + { + data.Add(new Row("r")); + } + }); + + await Task.Delay(30); + await ((IAsyncDisposable)cut.Instance).DisposeAsync(); + await Task.Delay(60); + stop.Cancel(); + await producer; + + var sent = JSInterop.Invocations + .Where(v => v.Identifier == "igSendMessage" && v.Arguments.Count > 1 && v.Arguments[0] as string == id) + .Select(v => (string)v.Arguments[1]!) + .ToList(); + var cleanup = sent.FindIndex(json => json.Contains("\"type\": \"cleanup\"")); + + Assert.True(cleanup >= 0, "disposal transmitted no cleanup message"); + Assert.Equal(sent.Count - 1, cleanup); + } } diff --git a/tests/IgniteUI.Blazor.Tests/Interop/InteropHarness.cs b/tests/IgniteUI.Blazor.Tests/Interop/InteropHarness.cs index 0e814779..4fb3960e 100644 --- a/tests/IgniteUI.Blazor.Tests/Interop/InteropHarness.cs +++ b/tests/IgniteUI.Blazor.Tests/Interop/InteropHarness.cs @@ -156,6 +156,13 @@ public void OnDispatcher(Action work) => /// Stubs the JS-side return value for invocations of . public abstract void SetupMethodResult(string methodName, InteropReturn result); + /// + /// Leaves invocations of unanswered until the returned action is + /// called with a reply. Until then the caller stays suspended on its send, so a test can + /// deliver something else - a return of its own, say - before the call resumes. + /// + public abstract Action WithholdMethodReply(string methodName); + /// /// Stubs the JS-side value for current-state reads of . /// How a read travels is implementation-specific (a "p:Name" method message diff --git a/tests/IgniteUI.Blazor.Tests/Interop/RendererMessageInteropHarness.cs b/tests/IgniteUI.Blazor.Tests/Interop/RendererMessageInteropHarness.cs index 0b6d8e52..b5907c67 100644 --- a/tests/IgniteUI.Blazor.Tests/Interop/RendererMessageInteropHarness.cs +++ b/tests/IgniteUI.Blazor.Tests/Interop/RendererMessageInteropHarness.cs @@ -222,6 +222,14 @@ public override void SetupMethodResult(string methodName, InteropReturn result) handler.SetResult(ToResultPayload(result)); } + public override Action WithholdMethodReply(string methodName) + { + _stubbedMethods.TryAdd(methodName, true); + var handler = _js.Setup(SendMessage, inv => MethodNameOf(inv) == methodName); + _methodHandlers[methodName] = handler; + return result => handler.SetResult(ToResultPayload(result)); + } + public override void SetupPropertyRead(string propertyName, InteropReturn result) => SetupMethodResult(PropertyReadMethodName(propertyName), result); diff --git a/tests/IgniteUI.Blazor.Tests/InteropThreadingTests.cs b/tests/IgniteUI.Blazor.Tests/InteropThreadingTests.cs index 8c074ef7..3df1183e 100644 --- a/tests/IgniteUI.Blazor.Tests/InteropThreadingTests.cs +++ b/tests/IgniteUI.Blazor.Tests/InteropThreadingTests.cs @@ -1,6 +1,7 @@ using System.Collections.ObjectModel; using Bunit; using IgniteUI.Blazor.Controls; +using IgniteUI.Blazor.Tests.Interop; namespace IgniteUI.Blazor.Tests; @@ -39,4 +40,29 @@ await Task.Run(() => // drains interleaving - that is what holding the lock across the whole drain is for. Assert.Equal(Enumerable.Range(0, Rows), Interop.DataItemInsertions(Interop.ContainerIdOf(cut), Rows)); } + + [Fact] + public async Task ConcurrentApiCalls_KeepInvocationBookkeepingIntact() + { + Interop.PrimeReady(); + Interop.SetupMethodResult("show", InteropReturn.Bool(true)); + var cut = Render(); + + // Unsynchronised, concurrent calls corrupt the maps pairing an invocation with its return. + // Sending from eight threads is only safe here because the queue's lock also covers the + // send, which is where bUnit records the invocation - see InteropHarness.OnDispatcher. + await Task.WhenAll(Enumerable.Range(0, 8).Select(_ => Task.Run(async () => + { + for (var i = 0; i < 2000; i++) + { + Assert.True(await cut.Instance.ShowAsync()); + } + }))); + + // Every call also has to have been given an id of its own: two sharing one collide in + // those maps, and each still completes its own local task, so nothing above would notice. + var ids = Interop.CallsOf("show", Interop.ContainerIdOf(cut)).Select(c => c.InvokeId).ToList(); + Assert.Equal(8 * 2000, ids.Count); + Assert.Equal(ids.Count, ids.Distinct().Count()); + } } diff --git a/tests/IgniteUI.Blazor.Tests/MethodInteropTests.cs b/tests/IgniteUI.Blazor.Tests/MethodInteropTests.cs index d40198cd..0365dc0d 100644 --- a/tests/IgniteUI.Blazor.Tests/MethodInteropTests.cs +++ b/tests/IgniteUI.Blazor.Tests/MethodInteropTests.cs @@ -27,4 +27,24 @@ public async Task DeferredReturn_StaysPendingUntilJsDeliversTheResult() Assert.True(await pending); } + + [Fact] + public async Task ReturnDeliveredBeforeTheCallRegisters_StillCompletesIt() + { + Interop.PrimeReady(); + // Held open so the return arrives while the call is still suspended on its send, with + // nothing registered for it yet; the promise reply then sends it looking for a stored one. + var reply = Interop.WithholdMethodReply("toggle"); + var cut = Render(); + + var pending = Interop.OnDispatcher(cut.Instance.ToggleAsync); + Interop.CompleteDeferred(Interop.RequireCall("toggle"), InteropReturn.Bool(true)); + reply(InteropReturn.Deferred); + + // A dropped return never completes, so this is bounded to fail rather than hang the run. + Assert.True( + await Task.WhenAny(pending, Task.Delay(TimeSpan.FromSeconds(10))) == pending, + "the call never completed - the return that arrived before it registered was dropped"); + Assert.True(await pending); + } }