Handle overlapping events (#1655)

* Add failing unit test to demonstrate overlapping events bug

* Handle overlapping events

* Make RemoteRenderer.UpdateDisplay's return task not complete until client sends explict ACK

* CR: Rename UpdateDisplay to UpdateDisplayAsync

* CR: Fix namespace

* CR: Catch synchronous SendAsync exceptions (if they can happen)

benchmarks/Microsoft.AspNetCore.Blazor.Performance/RenderTreeDiffBuilderBenchmark.cs

@@ -1,8 +1,9 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
 using System.Linq;
+using System.Threading.Tasks;
 using BenchmarkDotNet.Attributes;
 using Microsoft.AspNetCore.Blazor.Components;
 using Microsoft.AspNetCore.Blazor.Rendering;
@@ -92,9 +93,8 @@ namespace Microsoft.AspNetCore.Blazor.Performance
             {
             }
 
-            protected override void UpdateDisplay(in RenderBatch renderBatch)
+            protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
-            {
+                => Task.CompletedTask;
-            }
         }
 
         private class TestServiceProvider : IServiceProvider

src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Boot.Server.ts

@@ -23,8 +23,15 @@ function boot() {
     .build();
 
   connection.on('JS.BeginInvokeJS', DotNet.jsCallDispatcher.beginInvokeJSFromDotNet);
-  connection.on('JS.RenderBatch', (browserRendererId: number, batchData: Uint8Array) => {
+  connection.on('JS.RenderBatch', (browserRendererId: number, renderId: number, batchData: Uint8Array) => {
-    renderBatch(browserRendererId, new OutOfProcessRenderBatch(batchData));
+    try {
+      renderBatch(browserRendererId, new OutOfProcessRenderBatch(batchData));
+      connection.send('OnRenderCompleted', renderId, null);
+    } catch (ex) {
+      // If there's a rendering exception, notify server *and* throw on client
+      connection.send('OnRenderCompleted', renderId, ex.toString());
+      throw ex;
+    }
   });
 
   connection.on('JS.Error', unhandledError);

src/Microsoft.AspNetCore.Blazor.Browser/Rendering/BrowserRenderer.cs

@@ -7,6 +7,7 @@ using Microsoft.AspNetCore.Blazor.Rendering;
 using Microsoft.JSInterop;
 using Mono.WebAssembly.Interop;
 using System;
+using System.Threading.Tasks;
 
 namespace Microsoft.AspNetCore.Blazor.Browser.Rendering
 {
@@ -64,8 +65,8 @@ namespace Microsoft.AspNetCore.Blazor.Browser.Rendering
             var componentId = AssignRootComponentId(component);
 
             // The only reason we're calling this synchronously is so that, if it throws,
-            // we get the exception back *before* attempting the first UpdateDisplay
+            // we get the exception back *before* attempting the first UpdateDisplayAsync
-            // (otherwise the logged exception will come from UpdateDisplay instead of here)
+            // (otherwise the logged exception will come from UpdateDisplayAsync instead of here)
             // When implementing support for out-of-process runtimes, we'll need to call this
             // asynchronously and ensure we surface any exceptions correctly.
             ((IJSInProcessRuntime)JSRuntime.Current).Invoke<object>(
@@ -86,7 +87,7 @@ namespace Microsoft.AspNetCore.Blazor.Browser.Rendering
         }
 
         /// <inheritdoc />
-        protected override void UpdateDisplay(in RenderBatch batch)
+        protected override Task UpdateDisplayAsync(in RenderBatch batch)
         {
             if (JSRuntime.Current is MonoWebAssemblyJSRuntime mono)
             {
@@ -94,12 +95,12 @@ namespace Microsoft.AspNetCore.Blazor.Browser.Rendering
                     "Blazor._internal.renderBatch",
                     _browserRendererId,
                     batch);
+                return Task.CompletedTask;
             }
             else
             {
-                // When implementing support for an out-of-process JS runtime, we'll need to
+                // This renderer is not used for server-side Blazor.
-                // do something here to serialize and transmit the RenderBatch efficiently.
+                throw new NotImplementedException($"{nameof(BrowserRenderer)} is supported only with in-process JS runtimes.");
-                throw new NotImplementedException("TODO: Support BrowserRenderer.UpdateDisplay on other runtimes.");
             }
         }
     }

src/Microsoft.AspNetCore.Blazor.Server/BlazorHub.cs

@@ -78,6 +78,14 @@ namespace Microsoft.AspNetCore.Blazor.Server
             EnsureCircuitHost().BeginInvokeDotNetFromJS(callId, assemblyName, methodIdentifier, dotNetObjectId, argsJson);
         }
 
+        /// <summary>
+        /// Intended for framework use only. Applications should not call this method directly.
+        /// </summary>
+        public void OnRenderCompleted(long renderId, string errorMessageOrNull)
+        {
+            EnsureCircuitHost().Renderer.OnRenderCompleted(renderId, errorMessageOrNull);
+        }
+
         private async void CircuitHost_UnhandledException(object sender, UnhandledExceptionEventArgs e)
         {
             var circuitHost = (CircuitHost)sender;

src/Microsoft.AspNetCore.Blazor.Server/Circuits/AutoCancelTaskCompletionSource.cs

@@ -0,0 +1,45 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+using System.Threading;
+using System.Threading.Tasks;
+
+namespace Microsoft.AspNetCore.Blazor.Server.Circuits
+{
+    /// <summary>
+    /// Behaves like a <see cref="TaskCompletionSource{T}"/>, but automatically times out
+    /// the underlying task after a given period if not already completed.
+    /// </summary>
+    internal class AutoCancelTaskCompletionSource<T>
+    {
+        private readonly TaskCompletionSource<T> _completionSource;
+        private readonly CancellationTokenSource _timeoutSource;
+
+        public AutoCancelTaskCompletionSource(int timeoutMilliseconds)
+        {
+            _completionSource = new TaskCompletionSource<T>();
+            _timeoutSource = new CancellationTokenSource();
+            _timeoutSource.CancelAfter(timeoutMilliseconds);
+            _timeoutSource.Token.Register(() => _completionSource.TrySetCanceled());
+        }
+
+        public Task Task => _completionSource.Task;
+
+        public void TrySetResult(T result)
+        {
+            if (_completionSource.TrySetResult(result))
+            {
+                _timeoutSource.Dispose(); // We're not going to time out
+            }
+        }
+
+        public void TrySetException(Exception exception)
+        {
+            if (_completionSource.TrySetException(exception))
+            {
+                _timeoutSource.Dispose(); // We're not going to time out
+            }
+        }
+    }
+}

src/Microsoft.AspNetCore.Blazor.Server/Circuits/RemoteRenderer.cs

@@ -2,6 +2,8 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Collections.Concurrent;
+using System.Threading;
 using System.Threading.Tasks;
 using MessagePack;
 using Microsoft.AspNetCore.Blazor.Components;
@@ -14,10 +16,17 @@ namespace Microsoft.AspNetCore.Blazor.Browser.Rendering
 {
     internal class RemoteRenderer : Renderer
     {
+        // The purpose of the timeout is just to ensure server resources are released at some
+        // point if the client disconnects without sending back an ACK after a render
+        private const int TimeoutMilliseconds = 60 * 1000;
+
         private readonly int _id;
         private readonly IClientProxy _client;
         private readonly IJSRuntime _jsRuntime;
         private readonly RendererRegistry _rendererRegistry;
+        private readonly ConcurrentDictionary<long, AutoCancelTaskCompletionSource<object>> _pendingRenders
+            = new ConcurrentDictionary<long, AutoCancelTaskCompletionSource<object>>();
+        private long _nextRenderId = 1;
 
         /// <summary>
         /// Notifies when a rendering exception occured.
@@ -87,9 +96,8 @@ namespace Microsoft.AspNetCore.Blazor.Browser.Rendering
         }
 
         /// <inheritdoc />
-        protected override void UpdateDisplay(in RenderBatch batch)
+        protected override Task UpdateDisplayAsync(in RenderBatch batch)
         {
-            // Send the render batch to the client
             // Note that we have to capture the data as a byte[] synchronously here, because
             // SignalR's SendAsync can wait an arbitrary duration before serializing the params.
             // The RenderBatch buffer will get reused by subsequent renders, so we need to
@@ -97,8 +105,55 @@ namespace Microsoft.AspNetCore.Blazor.Browser.Rendering
             // TODO: Consider using some kind of array pool instead of allocating a new
             //       buffer on every render.
             var batchBytes = MessagePackSerializer.Serialize(batch, RenderBatchFormatterResolver.Instance);
-            var task = _client.SendAsync("JS.RenderBatch", _id, batchBytes);
+
-            CaptureAsyncExceptions(task);
+            // Prepare to track the render process with a timeout
+            var renderId = Interlocked.Increment(ref _nextRenderId);
+            var pendingRenderInfo = new AutoCancelTaskCompletionSource<object>(TimeoutMilliseconds);
+            _pendingRenders[renderId] = pendingRenderInfo;
+
+            // Send the render batch to the client
+            // If the "send" operation fails (synchronously or asynchronously), abort
+            // the whole render with that exception
+            try
+            {
+                _client.SendAsync("JS.RenderBatch", _id, renderId, batchBytes).ContinueWith(sendTask =>
+                {
+                    if (sendTask.IsFaulted)
+                    {
+                        pendingRenderInfo.TrySetException(sendTask.Exception);
+                    }
+                });
+            }
+            catch (Exception syncException)
+            {
+                pendingRenderInfo.TrySetException(syncException);
+            }
+
+            // When the render is completed (success, fail, or timeout), stop tracking it
+            return pendingRenderInfo.Task.ContinueWith(task =>
+            {
+                _pendingRenders.TryRemove(renderId, out var ignored);
+                if (task.IsFaulted)
+                {
+                    UnhandledException?.Invoke(this, task.Exception);
+                }
+            });
+        }
+
+        public void OnRenderCompleted(long renderId, string errorMessageOrNull)
+        {
+            if (_pendingRenders.TryGetValue(renderId, out var pendingRenderInfo))
+            {
+                if (errorMessageOrNull == null)
+                {
+                    pendingRenderInfo.TrySetResult(null);
+                }
+                else
+                {
+                    pendingRenderInfo.TrySetException(
+                        new RemoteRendererException(errorMessageOrNull));
+                }
+            }
         }
 
         private void CaptureAsyncExceptions(Task task)

src/Microsoft.AspNetCore.Blazor.Server/Circuits/RemoteRendererException.cs

@@ -0,0 +1,21 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+
+namespace Microsoft.AspNetCore.Blazor.Browser.Rendering
+{
+    /// <summary>
+    /// Represents an exception related to remote rendering.
+    /// </summary>
+    public class RemoteRendererException : Exception
+    {
+        /// <summary>
+        /// Constructs an instance of <see cref="RemoteRendererException"/>.
+        /// </summary>
+        /// <param name="message">The exception message.</param>
+        public RemoteRendererException(string message) : base(message)
+        {
+        }
+    }
+}

src/Microsoft.AspNetCore.Blazor/RenderTree/ArrayRange.cs

@@ -1,4 +1,4 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
@@ -41,5 +41,16 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree
         /// <inheritdoc />
         IEnumerator IEnumerable.GetEnumerator()
             => ((IEnumerable)new ArraySegment<T>(Array, 0, Count)).GetEnumerator();
+
+        /// <summary>
+        /// Creates a shallow clone of the instance.
+        /// </summary>
+        /// <returns></returns>
+        public ArrayRange<T> Clone()
+        {
+            var buffer = new T[Count];
+            System.Array.Copy(Array, buffer, Count);
+            return new ArrayRange<T>(buffer, Count);
+        }
     }
 }

src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs

@@ -5,6 +5,8 @@ using Microsoft.AspNetCore.Blazor.Components;
 using Microsoft.AspNetCore.Blazor.RenderTree;
 using System;
 using System.Collections.Generic;
+using System.Linq;
+using System.Threading.Tasks;
 
 namespace Microsoft.AspNetCore.Blazor.Rendering
 {
@@ -77,7 +79,8 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
         /// Updates the visible UI.
         /// </summary>
         /// <param name="renderBatch">The changes to the UI since the previous call.</param>
-        protected abstract void UpdateDisplay(in RenderBatch renderBatch);
+        /// <returns>A <see cref="Task"/> to represent the UI update process.</returns>
+        protected abstract Task UpdateDisplayAsync(in RenderBatch renderBatch);
 
         /// <summary>
         /// Notifies the specified component that an event has occurred.
@@ -169,6 +172,7 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
         private void ProcessRenderQueue()
         {
             _isBatchInProgress = true;
+            var updateDisplayTask = Task.CompletedTask;
 
             try
             {
@@ -180,12 +184,12 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
                 }
 
                 var batch = _batchBuilder.ToBatch();
-                UpdateDisplay(batch);
+                updateDisplayTask = UpdateDisplayAsync(batch);
                 InvokeRenderCompletedCalls(batch.UpdatedComponents);
             }
             finally
             {
-                RemoveEventHandlerIds(_batchBuilder.DisposedEventHandlerIds.ToRange());
+                RemoveEventHandlerIds(_batchBuilder.DisposedEventHandlerIds.ToRange(), updateDisplayTask);
                 _batchBuilder.Clear();
                 _isBatchInProgress = false;
             }
@@ -217,13 +221,31 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
             }
         }
 
-        private void RemoveEventHandlerIds(ArrayRange<int> eventHandlerIds)
+        private void RemoveEventHandlerIds(ArrayRange<int> eventHandlerIds, Task afterTask)
         {
-            var array = eventHandlerIds.Array;
+            if (eventHandlerIds.Count == 0)
-            var count = eventHandlerIds.Count;
-            for (var i = 0; i < count; i++)
             {
-                _eventBindings.Remove(array[i]);
+                return;
+            }
+
+            if (afterTask.IsCompleted)
+            {
+                var array = eventHandlerIds.Array;
+                var count = eventHandlerIds.Count;
+                for (var i = 0; i < count; i++)
+                {
+                    _eventBindings.Remove(array[i]);
+                }
+            }
+            else
+            {
+                // We need to delay the actual removal (e.g., until we've confirmed the client
+                // has processed the batch and hence can be sure not to reuse the handler IDs
+                // any further). We must clone the data because the underlying RenderBatchBuilder
+                // may be reused and hence modified by an unrelated subsequent batch.
+                var eventHandlerIdsClone = eventHandlerIds.Clone();
+                afterTask.ContinueWith(_ =>
+                    RemoveEventHandlerIds(eventHandlerIdsClone, Task.CompletedTask));
             }
         }
     }

test/Microsoft.AspNetCore.Blazor.Build.Test/RazorIntegrationTestBase.cs

@@ -8,6 +8,7 @@ using System.Linq;
 using System.Reflection;
 using System.Runtime.InteropServices;
 using System.Text;
+using System.Threading.Tasks;
 using Microsoft.AspNetCore.Blazor.Components;
 using Microsoft.AspNetCore.Blazor.Razor;
 using Microsoft.AspNetCore.Blazor.Rendering;
@@ -370,9 +371,10 @@ namespace Microsoft.AspNetCore.Blazor.Build.Test
             public void AttachComponent(IComponent component)
                 => AssignRootComponentId(component);
 
-            protected override void UpdateDisplay(in RenderBatch renderBatch)
+            protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
             {
                 LatestBatchReferenceFrames = renderBatch.ReferenceFrames.ToArray();
+                return Task.CompletedTask;
             }
         }
 

test/Microsoft.AspNetCore.Blazor.Test/RenderTreeBuilderTest.cs

@@ -7,6 +7,7 @@ using Microsoft.AspNetCore.Blazor.RenderTree;
 using Microsoft.AspNetCore.Blazor.Test.Helpers;
 using System;
 using System.Linq;
+using System.Threading.Tasks;
 using Xunit;
 
 namespace Microsoft.AspNetCore.Blazor.Test
@@ -1060,7 +1061,7 @@ namespace Microsoft.AspNetCore.Blazor.Test
             {
             }
 
-            protected override void UpdateDisplay(in RenderBatch renderBatch)
+            protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
                 => throw new NotImplementedException();
         }
     }

test/Microsoft.AspNetCore.Blazor.Test/RenderTreeDiffBuilderTest.cs

@@ -8,6 +8,7 @@ using Microsoft.AspNetCore.Blazor.Test.Helpers;
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading.Tasks;
 using Xunit;
 
 namespace Microsoft.AspNetCore.Blazor.Test
@@ -1530,9 +1531,8 @@ namespace Microsoft.AspNetCore.Blazor.Test
             {
             }
 
-            protected override void UpdateDisplay(in RenderBatch renderBatch)
+            protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
-            {
+                => Task.CompletedTask;
-            }
         }
 
         private class FakeComponent : IComponent

test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading.Tasks;
 using Microsoft.AspNetCore.Blazor.Components;
 using Microsoft.AspNetCore.Blazor.Rendering;
 using Microsoft.AspNetCore.Blazor.RenderTree;
@@ -1070,6 +1071,74 @@ namespace Microsoft.AspNetCore.Blazor.Test
             Assert.Equal(1, childComponents[2].OnAfterRenderCallCount); // Disposed
         }
 
+        [Fact]
+        public async Task CanTriggerEventHandlerDisposedInEarlierPendingBatch()
+        {
+            // This represents the scenario where the same event handler is being triggered
+            // rapidly, such as an input event while typing. It only applies to asynchronous
+            // batch updates, i.e., server-side Blazor.
+            // Sequence:
+            // 1. The client dispatches event X twice (say) in quick succession
+            // 2. The server receives the first instance, handles the event, and re-renders
+            //    some component. The act of re-rendering causes the old event handler to be
+            //    replaced by a new one, so the old one is flagged to be disposed.
+            // 3. The server receives the second instance. Even though the corresponding event
+            //    handler is flagged to be disposed, we have to still be able to find and
+            //    execute it without errors.
+
+            // Arrange
+            var renderer = new TestAsyncRenderer
+            {
+                NextUpdateDisplayReturnTask = Task.CompletedTask
+            };
+            var numEventsFired = 0;
+            EventComponent component = null;
+            Action<UIEventArgs> eventHandler = null;
+
+            eventHandler = _ =>
+            {
+                numEventsFired++;
+
+                // Replace the old event handler with a different one,
+                // (old the old handler ID will be disposed) then re-render.
+                component.OnTest = args => eventHandler(args);
+                component.TriggerRender();
+            };
+
+            component = new EventComponent { OnTest = eventHandler };
+            var componentId = renderer.AssignRootComponentId(component);
+            component.TriggerRender();
+
+            var eventHandlerId = renderer.Batches.Single()
+                .ReferenceFrames
+                .First(frame => frame.AttributeValue != null)
+                .AttributeEventHandlerId;
+
+            // Act/Assert 1: Event can be fired for the first time
+            var render1TCS = new TaskCompletionSource<object>();
+            renderer.NextUpdateDisplayReturnTask = render1TCS.Task;
+            renderer.DispatchEvent(componentId, eventHandlerId, new UIEventArgs());
+            Assert.Equal(1, numEventsFired);
+
+            // Act/Assert 2: *Same* event handler ID can be reused prior to completion of
+            // preceding UI update
+            var render2TCS = new TaskCompletionSource<object>();
+            renderer.NextUpdateDisplayReturnTask = render2TCS.Task;
+            renderer.DispatchEvent(componentId, eventHandlerId, new UIEventArgs());
+            Assert.Equal(2, numEventsFired);
+
+            // Act/Assert 3: After we complete the first UI update in which a given
+            // event handler ID is disposed, we can no longer reuse that event handler ID
+            render1TCS.SetResult(null);
+            await Task.Delay(100); // From here we can't see when the async disposal is completed. Just give it plenty of time (Task.Yield isn't enough).
+            var ex = Assert.Throws<ArgumentException>(() =>
+            {
+                renderer.DispatchEvent(componentId, eventHandlerId, new UIEventArgs());
+            });
+            Assert.Equal($"There is no event handler with ID {eventHandlerId}", ex.Message);
+            Assert.Equal(2, numEventsFired);
+        }
+
         private class NoOpRenderer : Renderer
         {
             public NoOpRenderer() : base(new TestServiceProvider())
@@ -1079,9 +1148,8 @@ namespace Microsoft.AspNetCore.Blazor.Test
             public new int AssignRootComponentId(IComponent component)
                 => base.AssignRootComponentId(component);
 
-            protected override void UpdateDisplay(in RenderBatch renderBatch)
+            protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
-            {
+                => Task.CompletedTask;
-            }
         }
 
         private class TestComponent : IComponent
@@ -1329,5 +1397,16 @@ namespace Microsoft.AspNetCore.Blazor.Test
             {
             }
         }
+
+        class TestAsyncRenderer : TestRenderer
+        {
+            public Task NextUpdateDisplayReturnTask { get; set; }
+
+            protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
+            {
+                base.UpdateDisplayAsync(renderBatch);
+                return NextUpdateDisplayReturnTask;
+            }
+        }
     }
 }

test/Microsoft.AspnetCore.Blazor.Server.Test/Circuits/RenderBatchWriterTest.cs

@@ -10,6 +10,7 @@ using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Text;
+using System.Threading.Tasks;
 using Xunit;
 
 namespace Microsoft.AspNetCore.Blazor.Server
@@ -376,7 +377,7 @@ namespace Microsoft.AspNetCore.Blazor.Server
             {
             }
 
-            protected override void UpdateDisplay(in RenderBatch renderBatch)
+            protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
                 => throw new NotImplementedException();
         }
     }

test/shared/TestRenderer.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading.Tasks;
 using Microsoft.AspNetCore.Blazor.Components;
 using Microsoft.AspNetCore.Blazor.Rendering;
 
@@ -33,7 +34,7 @@ namespace Microsoft.AspNetCore.Blazor.Test.Helpers
         public T InstantiateComponent<T>() where T : IComponent
             => (T)InstantiateComponent(typeof(T));
 
-        protected override void UpdateDisplay(in RenderBatch renderBatch)
+        protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
         {
             OnUpdateDisplay?.Invoke(renderBatch);
 
@@ -49,6 +50,10 @@ namespace Microsoft.AspNetCore.Blazor.Test.Helpers
             // Clone other data, as underlying storage will get reused by later batches
             capturedBatch.ReferenceFrames = renderBatch.ReferenceFrames.ToArray();
             capturedBatch.DisposedComponentIDs = renderBatch.DisposedComponentIDs.ToList();
+
+            // This renderer updates the UI synchronously, like the WebAssembly one.
+            // To test async UI updates, subclass TestRenderer and override UpdateDisplayAsync.
+            return Task.CompletedTask;
         }
     }
 }