fix: method invoke track & message queue at teardown concurrency guards

src/componentsBase/BaseRendererControl.cs

@@ -1126,7 +1126,11 @@ private IgbJsonContext SerializerContext
             var ret = await SendMessageImmediate(m);
 
             TaskCompletionSource<object?> tcs = new TaskCompletionSource<object?>();
-            _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<object?> 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<object?> 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<object?>? 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)
         /// <inheritdoc />
         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

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;
 
+/// <summary>
+/// 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.
+/// </summary>
+// 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";
+}
+
 /// <summary>
 /// Tests around <see cref="BaseRendererControl.DisposeAsync"/> — 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.
 /// </summary>
+[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<string?> MessageTypesFor(string containerId)
+    {
+        var types = new List<string?>();
+        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<Row>();
+        var cut = Render<IgbCombo<Row>>(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<Row>();
+        var cut = Render<IgbCombo<Row>>(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);
+    }
 }

tests/IgniteUI.Blazor.Tests/Interop/InteropHarness.cs

@@ -156,6 +156,13 @@ public void OnDispatcher(Action work) =>
     /// <summary>Stubs the JS-side return value for invocations of <paramref name="methodName"/>.</summary>
     public abstract void SetupMethodResult(string methodName, InteropReturn result);
 
+    /// <summary>
+    /// Leaves invocations of <paramref name="methodName"/> 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.
+    /// </summary>
+    public abstract Action<InteropReturn> WithholdMethodReply(string methodName);
+
     /// <summary>
     /// Stubs the JS-side value for current-state reads of <paramref name="propertyName"/>.
     /// How a read travels is implementation-specific (a <c>"p:Name"</c> method message

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<InteropReturn> WithholdMethodReply(string methodName)
+    {
+        _stubbedMethods.TryAdd(methodName, true);
+        var handler = _js.Setup<object>(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);
 

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<IgbSnackbar>();
+
+        // 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());
+    }
 }

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<IgbBanner>();
+
+        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);
+    }
 }