diff --git a/src/Microsoft.AspNetCore.Blazor/Components/RenderHandle.cs b/src/Microsoft.AspNetCore.Blazor/Components/RenderHandle.cs index 13d3b22804..d3e3bb4c0c 100644 --- a/src/Microsoft.AspNetCore.Blazor/Components/RenderHandle.cs +++ b/src/Microsoft.AspNetCore.Blazor/Components/RenderHandle.cs @@ -39,7 +39,7 @@ namespace Microsoft.AspNetCore.Blazor.Components throw new InvalidOperationException("The render handle is not yet assigned."); } - _renderer.AddToRenderQueue(new RenderQueueEntry(_componentId, renderFragment)); + _renderer.AddToRenderQueue(_componentId, renderFragment); } } } diff --git a/src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs b/src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs index 6b4bea78e4..feaaf519d2 100644 --- a/src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs +++ b/src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs @@ -19,6 +19,7 @@ namespace Microsoft.AspNetCore.Blazor.Rendering private readonly Renderer _renderer; private RenderTreeBuilder _renderTreeBuilderCurrent; private RenderTreeBuilder _renderTreeBuilderPrevious; + private bool _componentWasDisposed; /// /// Constructs an instance of . @@ -37,6 +38,13 @@ namespace Microsoft.AspNetCore.Blazor.Rendering public void RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment renderFragment) { + // A component might be in the render queue already before getting disposed by an + // earlier entry in the render queue. In that case, rendering is a no-op. + if (_componentWasDisposed) + { + return; + } + // Swap the old and new tree builders (_renderTreeBuilderCurrent, _renderTreeBuilderPrevious) = (_renderTreeBuilderPrevious, _renderTreeBuilderCurrent); @@ -54,6 +62,8 @@ namespace Microsoft.AspNetCore.Blazor.Rendering public void DisposeInBatch(RenderBatchBuilder batchBuilder) { + _componentWasDisposed = true; + // TODO: Handle components throwing during dispose. Shouldn't break the whole render batch. if (_component is IDisposable disposable) { diff --git a/src/Microsoft.AspNetCore.Blazor/Rendering/RenderQueueEntry.cs b/src/Microsoft.AspNetCore.Blazor/Rendering/RenderQueueEntry.cs index b6e1a6cb98..565f1466d4 100644 --- a/src/Microsoft.AspNetCore.Blazor/Rendering/RenderQueueEntry.cs +++ b/src/Microsoft.AspNetCore.Blazor/Rendering/RenderQueueEntry.cs @@ -8,12 +8,12 @@ namespace Microsoft.AspNetCore.Blazor.Rendering { internal readonly struct RenderQueueEntry { - public readonly int ComponentId; + public readonly ComponentState ComponentState; public readonly RenderFragment RenderFragment; - public RenderQueueEntry(int componentId, RenderFragment renderFragment) + public RenderQueueEntry(ComponentState componentState, RenderFragment renderFragment) { - ComponentId = componentId; + ComponentState = componentState; RenderFragment = renderFragment ?? throw new ArgumentNullException(nameof(renderFragment)); } } diff --git a/src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs b/src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs index e746696d15..e752c85e3b 100644 --- a/src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs +++ b/src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs @@ -71,7 +71,18 @@ namespace Microsoft.AspNetCore.Blazor.Rendering { if (_eventHandlersById.TryGetValue(eventHandlerId, out var handler)) { - GetRequiredComponentState(componentId).DispatchEvent(handler, eventArgs); + // The event handler might request multiple renders in sequence. Capture them + // all in a single batch. + try + { + _isBatchInProgress = true; + GetRequiredComponentState(componentId).DispatchEvent(handler, eventArgs); + } + finally + { + _isBatchInProgress = false; + ProcessRenderQueue(); + } } else { @@ -103,9 +114,18 @@ namespace Microsoft.AspNetCore.Blazor.Rendering frame = frame.WithAttributeEventHandlerId(id); } - internal void AddToRenderQueue(RenderQueueEntry renderQueueEntry) + internal void AddToRenderQueue(int componentId, RenderFragment renderFragment) { - _batchBuilder.ComponentRenderQueue.Enqueue(renderQueueEntry); + var componentState = GetOptionalComponentState(componentId); + if (componentState == null) + { + // If the component was already disposed, then its render handle trying to + // queue a render is a no-op. + return; + } + + _batchBuilder.ComponentRenderQueue.Enqueue( + new RenderQueueEntry(componentState, renderFragment)); if (!_isBatchInProgress) { @@ -118,6 +138,11 @@ namespace Microsoft.AspNetCore.Blazor.Rendering ? componentState : throw new ArgumentException($"The renderer does not have a component with ID {componentId}."); + private ComponentState GetOptionalComponentState(int componentId) + => _componentStateById.TryGetValue(componentId, out var componentState) + ? componentState + : null; + private void ProcessRenderQueue() { _isBatchInProgress = true; @@ -143,8 +168,7 @@ namespace Microsoft.AspNetCore.Blazor.Rendering private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry) { - var componentId = renderQueueEntry.ComponentId; - GetRequiredComponentState(componentId) + renderQueueEntry.ComponentState .RenderIntoBatch(_batchBuilder, renderQueueEntry.RenderFragment); // Process disposal queue now in case it causes further component renders to be enqueued diff --git a/test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs b/test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs index 6d396fed6e..5a2c34f2e9 100644 --- a/test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs +++ b/test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs @@ -450,14 +450,23 @@ namespace Microsoft.AspNetCore.Blazor.Test .Where(frame => frame.FrameType == RenderTreeFrameType.Component) .Select(frame => frame.ComponentId) .ToList(); + var childComponent3 = batch.ReferenceFrames.Where(f => f.ComponentId == 3) + .Single().Component; Assert.Equal(new[] { 1, 2 }, childComponentIds); + Assert.IsType(childComponent3); // Act: Second render firstRender = false; component.TriggerRender(); // Assert: Applicable children are included in disposal list + Assert.Equal(2, renderer.Batches.Count); Assert.Equal(new[] { 1, 3 }, renderer.Batches[1].DisposedComponentIDs); + + // Act/Assert: If a disposed component requests a render, it's a no-op + ((FakeComponent)childComponent3).RenderHandle.Render(builder + => throw new NotImplementedException("Should not be invoked")); + Assert.Equal(2, renderer.Batches.Count); } [Fact] @@ -612,6 +621,85 @@ namespace Microsoft.AspNetCore.Blazor.Test Assert.Equal(1, eventCount); } + [Fact] + public void AllRendersTriggeredSynchronouslyDuringEventHandlerAreHandledAsSingleBatch() + { + // Arrange: A root component with a child whose event handler explicitly queues + // a re-render of both the root component and the child + var renderer = new TestRenderer(); + var eventCount = 0; + TestComponent rootComponent = null; + EventComponent childComponent = null; + rootComponent = new TestComponent(builder => + { + builder.AddContent(0, "Child event count: " + eventCount); + builder.OpenComponent(1); + builder.AddAttribute(2, nameof(EventComponent.Handler), args => + { + eventCount++; + rootComponent.TriggerRender(); + childComponent.TriggerRender(); + }); + builder.CloseComponent(); + }); + var rootComponentId = renderer.AssignComponentId(rootComponent); + rootComponent.TriggerRender(); + var origBatchReferenceFrames = renderer.Batches.Single().ReferenceFrames; + var childComponentFrame = origBatchReferenceFrames + .Single(f => f.Component is EventComponent); + var childComponentId = childComponentFrame.ComponentId; + childComponent = (EventComponent)childComponentFrame.Component; + var origEventHandlerId = origBatchReferenceFrames + .Where(f => f.FrameType == RenderTreeFrameType.Attribute) + .Last(f => f.AttributeEventHandlerId != 0) + .AttributeEventHandlerId; + Assert.Single(renderer.Batches); + + // Act + renderer.DispatchEvent(childComponentId, origEventHandlerId, args: null); + + // Assert + Assert.Equal(2, renderer.Batches.Count); + var batch = renderer.Batches.Last(); + Assert.Collection(batch.DiffsInOrder, + diff => + { + // First we triggered the root component to re-render + Assert.Equal(rootComponentId, diff.ComponentId); + Assert.Collection(diff.Edits, edit => + { + Assert.Equal(RenderTreeEditType.UpdateText, edit.Type); + AssertFrame.Text( + batch.ReferenceFrames[edit.ReferenceFrameIndex], + "Child event count: 1"); + }); + }, + diff => + { + // Then the root re-render will have triggered an update to the child + Assert.Equal(childComponentId, diff.ComponentId); + Assert.Collection(diff.Edits, edit => + { + Assert.Equal(RenderTreeEditType.UpdateText, edit.Type); + AssertFrame.Text( + batch.ReferenceFrames[edit.ReferenceFrameIndex], + "Render count: 2"); + }); + }, + diff => + { + // Finally we explicitly requested a re-render of the child + Assert.Equal(childComponentId, diff.ComponentId); + Assert.Collection(diff.Edits, edit => + { + Assert.Equal(RenderTreeEditType.UpdateText, edit.Type); + AssertFrame.Text( + batch.ReferenceFrames[edit.ReferenceFrameIndex], + "Render count: 3"); + }); + }); + } + [Fact] public void ComponentCannotTriggerRenderBeforeRenderHandleAssigned() { @@ -721,6 +809,60 @@ namespace Microsoft.AspNetCore.Blazor.Test Assert.Empty(diff4.Edits); } + [Fact] + public void QueuedRenderIsSkippedIfComponentWasAlreadyDisposedInSameBatch() + { + // Arrange + var renderer = new TestRenderer(); + var shouldRenderChild = true; + TestComponent component = null; + component = new TestComponent(builder => + { + builder.AddContent(0, "Some frame so the child isn't at position zero"); + if (shouldRenderChild) + { + builder.OpenComponent(1); + builder.AddAttribute(2, nameof(RendersSelfAfterEventComponent.OnClick), (Action)(() => + { + // First we queue (1) a re-render of the root component, then the child component + // will queue (2) its own re-render. But by the time (1) completes, the child will + // have been disposed, even though (2) is still in the queue + shouldRenderChild = false; + component.TriggerRender(); + })); + builder.CloseComponent(); + } + }); + + var componentId = renderer.AssignComponentId(component); + component.TriggerRender(); + var childComponentId = renderer.Batches.Single() + .ReferenceFrames + .Where(f => f.ComponentId != 0) + .Single() + .ComponentId; + var origEventHandlerId = renderer.Batches.Single() + .ReferenceFrames + .Where(f => f.FrameType == RenderTreeFrameType.Attribute) + .Single(f => f.AttributeEventHandlerId != 0) + .AttributeEventHandlerId; + + // Act + // The fact that there's no error here is the main thing we're testing + renderer.DispatchEvent(childComponentId, origEventHandlerId, args: null); + + // Assert: correct render result + var newBatch = renderer.Batches.Skip(1).Single(); + Assert.Equal(1, newBatch.DisposedComponentIDs.Count); + Assert.Equal(1, newBatch.DiffsByComponentId.Count); + Assert.Collection(newBatch.DiffsByComponentId[componentId].Single().Edits, + edit => + { + Assert.Equal(RenderTreeEditType.RemoveFrame, edit.Type); + Assert.Equal(1, edit.SiblingIndex); + }); + } + private class NoOpRenderer : Renderer { public new int AssignComponentId(IComponent component) @@ -824,10 +966,10 @@ namespace Microsoft.AspNetCore.Blazor.Test public int IntProperty { get; set; } public string StringProperty { get; set; } public object ObjectProperty { get; set; } + public RenderHandle RenderHandle { get; private set; } public void Init(RenderHandle renderHandle) - { - } + => RenderHandle = renderHandle; public void SetParameters(ParameterCollection parameters) => parameters.AssignToProperties(this); @@ -837,6 +979,7 @@ namespace Microsoft.AspNetCore.Blazor.Test { public UIEventHandler Handler { get; set; } public bool SkipElement { get; set; } + private int renderCount = 0; protected override void BuildRenderTree(RenderTreeBuilder builder) { @@ -853,6 +996,7 @@ namespace Microsoft.AspNetCore.Blazor.Test builder.CloseElement(); } builder.CloseElement(); + builder.AddContent(4, $"Render count: {++renderCount}"); } public void HandleEvent(UIEventHandler handler, UIEventArgs args) @@ -901,6 +1045,36 @@ namespace Microsoft.AspNetCore.Blazor.Test } } + private class RendersSelfAfterEventComponent : IComponent, IHandleEvent + { + public Action OnClick { get; set; } + + private RenderHandle _renderHandle; + + public void Init(RenderHandle renderHandle) + => _renderHandle = renderHandle; + + public void SetParameters(ParameterCollection parameters) + { + parameters.AssignToProperties(this); + Render(); + } + + public void HandleEvent(UIEventHandler handler, UIEventArgs args) + { + handler(args); + Render(); + } + + private void Render() + => _renderHandle.Render(builder => + { + builder.OpenElement(0, "my button"); + builder.AddAttribute(1, "my click handler", eventArgs => OnClick()); + builder.CloseElement(); + }); + } + private class MultiRendererComponent : IComponent { private readonly List _renderHandles