From a2980d0f1d110f56b77700cf4294ac3494258d0f Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 24 May 2023 11:35:13 +0100 Subject: [PATCH 1/3] Ensure ComponentState is always disposed --- .../Components/src/PublicAPI.Unshipped.txt | 2 +- .../Components/src/RenderTree/Renderer.cs | 38 ++++++------------- .../src/Rendering/ComponentState.cs | 20 +++++++--- 3 files changed, 28 insertions(+), 32 deletions(-) diff --git a/src/Components/Components/src/PublicAPI.Unshipped.txt b/src/Components/Components/src/PublicAPI.Unshipped.txt index 6048b3a28d79..37c83450648c 100644 --- a/src/Components/Components/src/PublicAPI.Unshipped.txt +++ b/src/Components/Components/src/PublicAPI.Unshipped.txt @@ -19,6 +19,7 @@ Microsoft.AspNetCore.Components.ParameterView.ToDictionary() -> System.Collectio Microsoft.AspNetCore.Components.RenderHandle.DispatchExceptionAsync(System.Exception! exception) -> System.Threading.Tasks.Task! *REMOVED*Microsoft.AspNetCore.Components.NavigationManager.ToAbsoluteUri(string! relativeUri) -> System.Uri! Microsoft.AspNetCore.Components.NavigationManager.ToAbsoluteUri(string? relativeUri) -> System.Uri! +Microsoft.AspNetCore.Components.Rendering.ComponentState.DisposeAsync() -> System.Threading.Tasks.ValueTask Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder.SetEventHandlerName(string! eventHandlerName) -> void *REMOVED*Microsoft.AspNetCore.Components.RouteData.RouteData(System.Type! pageType, System.Collections.Generic.IReadOnlyDictionary! routeValues) -> void *REMOVED*Microsoft.AspNetCore.Components.RouteData.RouteValues.get -> System.Collections.Generic.IReadOnlyDictionary! @@ -34,7 +35,6 @@ Microsoft.AspNetCore.Components.Rendering.ComponentState Microsoft.AspNetCore.Components.Rendering.ComponentState.Component.get -> Microsoft.AspNetCore.Components.IComponent! Microsoft.AspNetCore.Components.Rendering.ComponentState.ComponentId.get -> int Microsoft.AspNetCore.Components.Rendering.ComponentState.ComponentState(Microsoft.AspNetCore.Components.RenderTree.Renderer! renderer, int componentId, Microsoft.AspNetCore.Components.IComponent! component, Microsoft.AspNetCore.Components.Rendering.ComponentState? parentComponentState) -> void -Microsoft.AspNetCore.Components.Rendering.ComponentState.Dispose() -> void Microsoft.AspNetCore.Components.Rendering.ComponentState.ParentComponentState.get -> Microsoft.AspNetCore.Components.Rendering.ComponentState? Microsoft.AspNetCore.Components.RenderTree.Renderer.GetComponentState(int componentId) -> Microsoft.AspNetCore.Components.Rendering.ComponentState! Microsoft.AspNetCore.Components.Sections.SectionContent diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 001454f09b02..8be1ea9ed14b 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -1080,39 +1080,25 @@ protected virtual void Dispose(bool disposing) { Log.DisposingComponent(_logger, componentState); - // Components shouldn't need to implement IAsyncDisposable and IDisposable simultaneously, - // but in case they do, we prefer the async overload since we understand the sync overload - // is implemented for more "constrained" scenarios. - // Component authors are responsible for their IAsyncDisposable implementations not taking - // forever. - if (componentState.Component is IAsyncDisposable asyncDisposable) + try { - try + var task = componentState.DisposeAsync(); + if (task.IsCompletedSuccessfully) { - var task = asyncDisposable.DisposeAsync(); - if (!task.IsCompletedSuccessfully) - { - asyncDisposables ??= new(); - asyncDisposables.Add(task.AsTask()); - } + // If it's a IValueTaskSource backed ValueTask, + // inform it its result has been read so it can reset + task.GetAwaiter().GetResult(); } - catch (Exception exception) + else { - exceptions ??= new List(); - exceptions.Add(exception); + asyncDisposables ??= new(); + asyncDisposables.Add(task.AsTask()); } } - else if (componentState.Component is IDisposable disposable) + catch (Exception exception) { - try - { - componentState.Dispose(); - } - catch (Exception exception) - { - exceptions ??= new List(); - exceptions.Add(exception); - } + exceptions ??= new List(); + exceptions.Add(exception); } } diff --git a/src/Components/Components/src/Rendering/ComponentState.cs b/src/Components/Components/src/Rendering/ComponentState.cs index 8252c36a1a6d..a898a9380f8c 100644 --- a/src/Components/Components/src/Rendering/ComponentState.cs +++ b/src/Components/Components/src/Rendering/ComponentState.cs @@ -13,7 +13,7 @@ namespace Microsoft.AspNetCore.Components.Rendering; /// detail of . /// [DebuggerDisplay($"{{{nameof(GetDebuggerDisplay)}(),nq}}")] -public class ComponentState : IDisposable +public class ComponentState : IAsyncDisposable { private readonly Renderer _renderer; private readonly IReadOnlyList _cascadingParameters; @@ -254,15 +254,25 @@ private void RemoveCascadingParameterSubscriptions() } /// - /// Disposes this instance. + /// Disposes this instance and its associated component. /// - public void Dispose() + public ValueTask DisposeAsync() { DisposeBuffers(); - if (Component is IDisposable disposable) + // Components shouldn't need to implement IAsyncDisposable and IDisposable simultaneously, + // but in case they do, we prefer the async overload since we understand the sync overload + // is implemented for more "constrained" scenarios. + // Component authors are responsible for their IAsyncDisposable implementations not taking + // forever. + if (Component is IAsyncDisposable asyncDisposable) { - disposable.Dispose(); + return asyncDisposable.DisposeAsync(); + } + else + { + (Component as IDisposable)?.Dispose(); + return ValueTask.CompletedTask; } } From 72f53eaac712ad19ff0a2ed035bcf80d344c04e4 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 24 May 2023 14:25:01 +0100 Subject: [PATCH 2/3] Tests plus further unification of disposal code paths --- .../Components/src/PublicAPI.Unshipped.txt | 2 +- .../Components/src/RenderTree/Renderer.cs | 29 +++++----- .../src/Rendering/ComponentState.cs | 53 ++----------------- .../Components/test/RendererTest.cs | 8 ++- src/Components/Shared/test/TestRenderer.cs | 34 ++++++++++++ 5 files changed, 55 insertions(+), 71 deletions(-) diff --git a/src/Components/Components/src/PublicAPI.Unshipped.txt b/src/Components/Components/src/PublicAPI.Unshipped.txt index 37c83450648c..a03797860d22 100644 --- a/src/Components/Components/src/PublicAPI.Unshipped.txt +++ b/src/Components/Components/src/PublicAPI.Unshipped.txt @@ -19,7 +19,6 @@ Microsoft.AspNetCore.Components.ParameterView.ToDictionary() -> System.Collectio Microsoft.AspNetCore.Components.RenderHandle.DispatchExceptionAsync(System.Exception! exception) -> System.Threading.Tasks.Task! *REMOVED*Microsoft.AspNetCore.Components.NavigationManager.ToAbsoluteUri(string! relativeUri) -> System.Uri! Microsoft.AspNetCore.Components.NavigationManager.ToAbsoluteUri(string? relativeUri) -> System.Uri! -Microsoft.AspNetCore.Components.Rendering.ComponentState.DisposeAsync() -> System.Threading.Tasks.ValueTask Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder.SetEventHandlerName(string! eventHandlerName) -> void *REMOVED*Microsoft.AspNetCore.Components.RouteData.RouteData(System.Type! pageType, System.Collections.Generic.IReadOnlyDictionary! routeValues) -> void *REMOVED*Microsoft.AspNetCore.Components.RouteData.RouteValues.get -> System.Collections.Generic.IReadOnlyDictionary! @@ -61,6 +60,7 @@ override Microsoft.AspNetCore.Components.EventCallback.GetHashCode() -> int override Microsoft.AspNetCore.Components.EventCallback.Equals(object? obj) -> bool override Microsoft.AspNetCore.Components.EventCallback.GetHashCode() -> int override Microsoft.AspNetCore.Components.EventCallback.Equals(object? obj) -> bool +virtual Microsoft.AspNetCore.Components.Rendering.ComponentState.DisposeAsync() -> System.Threading.Tasks.ValueTask virtual Microsoft.AspNetCore.Components.RenderTree.Renderer.AddPendingTask(Microsoft.AspNetCore.Components.Rendering.ComponentState? componentState, System.Threading.Tasks.Task! task) -> void virtual Microsoft.AspNetCore.Components.RenderTree.Renderer.CreateComponentState(int componentId, Microsoft.AspNetCore.Components.IComponent! component, Microsoft.AspNetCore.Components.Rendering.ComponentState? parentComponentState) -> Microsoft.AspNetCore.Components.Rendering.ComponentState! virtual Microsoft.AspNetCore.Components.RenderTree.Renderer.DispatchEventAsync(ulong eventHandlerId, Microsoft.AspNetCore.Components.RenderTree.EventFieldInfo? fieldInfo, System.EventArgs! eventArgs, bool quiesce) -> System.Threading.Tasks.Task! diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index 8be1ea9ed14b..187ffb6d8f79 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -876,28 +876,20 @@ private void ProcessDisposalQueueInExistingBatch() var disposeComponentId = _batchBuilder.ComponentDisposalQueue.Dequeue(); var disposeComponentState = GetRequiredComponentState(disposeComponentId); Log.DisposingComponent(_logger, disposeComponentState); - if (!(disposeComponentState.Component is IAsyncDisposable)) - { - if (!disposeComponentState.TryDisposeInBatch(_batchBuilder, out var exception)) - { - exceptions ??= new List(); - exceptions.Add(exception); - } - } - else + + try { - var result = disposeComponentState.DisposeInBatchAsync(_batchBuilder); - if (result.IsCompleted) + var disposalTask = disposeComponentState.DisposeInBatchAsync(_batchBuilder); + if (disposalTask.IsCompletedSuccessfully) { - if (!result.IsCompletedSuccessfully) - { - exceptions ??= new List(); - exceptions.Add(result.Exception); - } + // If it's a IValueTaskSource backed ValueTask, + // inform it its result has been read so it can reset + disposalTask.GetAwaiter().GetResult(); } else { // We set owningComponentState to null because we don't want exceptions during disposal to be recoverable + var result = disposalTask.AsTask(); AddToPendingTasksWithErrorHandling(GetHandledAsynchronousDisposalErrorsTask(result), owningComponentState: null); async Task GetHandledAsynchronousDisposalErrorsTask(Task result) @@ -913,6 +905,11 @@ async Task GetHandledAsynchronousDisposalErrorsTask(Task result) } } } + catch (Exception exception) + { + exceptions ??= new List(); + exceptions.Add(exception); + } _componentStateById.Remove(disposeComponentId); _componentStateByComponent.Remove(disposeComponentState.Component); diff --git a/src/Components/Components/src/Rendering/ComponentState.cs b/src/Components/Components/src/Rendering/ComponentState.cs index a898a9380f8c..81d9d117f047 100644 --- a/src/Components/Components/src/Rendering/ComponentState.cs +++ b/src/Components/Components/src/Rendering/ComponentState.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; -using System.Diagnostics.CodeAnalysis; using Microsoft.AspNetCore.Components.RenderTree; namespace Microsoft.AspNetCore.Components.Rendering; @@ -108,28 +107,6 @@ internal void RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment re batchBuilder.InvalidateParameterViews(); } - internal bool TryDisposeInBatch(RenderBatchBuilder batchBuilder, [NotNullWhen(false)] out Exception? exception) - { - _componentWasDisposed = true; - exception = null; - - try - { - if (Component is IDisposable disposable) - { - disposable.Dispose(); - } - } - catch (Exception ex) - { - exception = ex; - } - - CleanupComponentStateResources(batchBuilder); - - return exception == null; - } - private void CleanupComponentStateResources(RenderBatchBuilder batchBuilder) { // We don't expect these things to throw. @@ -256,8 +233,9 @@ private void RemoveCascadingParameterSubscriptions() /// /// Disposes this instance and its associated component. /// - public ValueTask DisposeAsync() + public virtual ValueTask DisposeAsync() { + _componentWasDisposed = true; DisposeBuffers(); // Components shouldn't need to implement IAsyncDisposable and IDisposable simultaneously, @@ -283,33 +261,10 @@ private void DisposeBuffers() _latestDirectParametersSnapshot?.Dispose(); } - internal Task DisposeInBatchAsync(RenderBatchBuilder batchBuilder) + internal ValueTask DisposeInBatchAsync(RenderBatchBuilder batchBuilder) { - _componentWasDisposed = true; - CleanupComponentStateResources(batchBuilder); - - try - { - var result = ((IAsyncDisposable)Component).DisposeAsync(); - if (result.IsCompletedSuccessfully) - { - // If it's a IValueTaskSource backed ValueTask, - // inform it its result has been read so it can reset - result.GetAwaiter().GetResult(); - return Task.CompletedTask; - } - else - { - // We know we are dealing with an exception that happened asynchronously, so return a task - // to the caller so that he can unwrap it. - return result.AsTask(); - } - } - catch (Exception e) - { - return Task.FromException(e); - } + return DisposeAsync(); } private string GetDebuggerDisplay() diff --git a/src/Components/Components/test/RendererTest.cs b/src/Components/Components/test/RendererTest.cs index 29dd39f23805..02ecce2bbdcd 100644 --- a/src/Components/Components/test/RendererTest.cs +++ b/src/Components/Components/test/RendererTest.cs @@ -2334,9 +2334,8 @@ public void RenderBatch_HandlesSynchronousExceptionsInAsyncDisposableComponents( // Outer component is still alive and not disposed. Assert.False(component.Disposed); - var aex = Assert.IsType(Assert.Single(renderer.HandledExceptions)); - var innerException = Assert.Single(aex.Flatten().InnerExceptions); - Assert.Same(exception1, innerException); + var aex = Assert.Single(renderer.HandledExceptions); + Assert.Same(exception1, aex); } [Fact] @@ -2493,8 +2492,7 @@ public void RenderBatch_ReportsSynchronousCancelationsAsErrors() // Outer component is still alive and not disposed. Assert.False(component.Disposed); - var aex = Assert.IsType(Assert.Single(renderer.HandledExceptions)); - Assert.IsType(Assert.Single(aex.Flatten().InnerExceptions)); + Assert.IsType(Assert.Single(renderer.HandledExceptions)); } [Fact] diff --git a/src/Components/Shared/test/TestRenderer.cs b/src/Components/Shared/test/TestRenderer.cs index 1cc98044ca7f..ff25193733a9 100644 --- a/src/Components/Shared/test/TestRenderer.cs +++ b/src/Components/Shared/test/TestRenderer.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Runtime.ExceptionServices; +using Microsoft.AspNetCore.Components.Rendering; using Microsoft.AspNetCore.Components.RenderTree; using Microsoft.Extensions.Logging.Abstractions; @@ -60,6 +61,8 @@ protected internal override bool ShouldTrackNamedEventHandlers() public Task NextRenderResultTask { get; set; } = Task.CompletedTask; + private HashSet UndisposedComponentStates { get; } = new(); + public new int AssignRootComponentId(IComponent component) => base.AssignRootComponentId(component); @@ -145,4 +148,35 @@ protected override Task UpdateDisplayAsync(in RenderBatch renderBatch) public new void ProcessPendingRender() => base.ProcessPendingRender(); + + protected override ComponentState CreateComponentState(int componentId, IComponent component, ComponentState parentComponentState) + => new TestRendererComponentState(this, componentId, component, parentComponentState); + + protected override void Dispose(bool disposing) + { + base.Dispose(disposing); + + if (UndisposedComponentStates.Count > 0) + { + throw new InvalidOperationException("Did not dispose all the ComponentState instances. This could lead to ArrayBuffer not returning buffers to its pool."); + } + } + + class TestRendererComponentState : ComponentState, IAsyncDisposable + { + private readonly TestRenderer _renderer; + + public TestRendererComponentState(Renderer renderer, int componentId, IComponent component, ComponentState parentComponentState) + : base(renderer, componentId, component, parentComponentState) + { + _renderer = (TestRenderer)renderer; + _renderer.UndisposedComponentStates.Add(this); + } + + public override ValueTask DisposeAsync() + { + _renderer.UndisposedComponentStates.Remove(this); + return base.DisposeAsync(); + } + } } From 9a735ab4f20cb50809da1c9c1736afdad85e8b91 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 24 May 2023 14:37:36 +0100 Subject: [PATCH 3/3] Tidy --- .../src/Rendering/ComponentState.cs | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/Components/Components/src/Rendering/ComponentState.cs b/src/Components/Components/src/Rendering/ComponentState.cs index 81d9d117f047..f7de9f106215 100644 --- a/src/Components/Components/src/Rendering/ComponentState.cs +++ b/src/Components/Components/src/Rendering/ComponentState.cs @@ -107,19 +107,6 @@ internal void RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment re batchBuilder.InvalidateParameterViews(); } - private void CleanupComponentStateResources(RenderBatchBuilder batchBuilder) - { - // We don't expect these things to throw. - RenderTreeDiffBuilder.DisposeFrames(batchBuilder, CurrentRenderTree.GetFrames()); - - if (_hasAnyCascadingParameterSubscriptions) - { - RemoveCascadingParameterSubscriptions(); - } - - DisposeBuffers(); - } - // Callers expect this method to always return a faulted task. internal Task NotifyRenderCompletedAsync() { @@ -263,7 +250,14 @@ private void DisposeBuffers() internal ValueTask DisposeInBatchAsync(RenderBatchBuilder batchBuilder) { - CleanupComponentStateResources(batchBuilder); + // We don't expect these things to throw. + RenderTreeDiffBuilder.DisposeFrames(batchBuilder, CurrentRenderTree.GetFrames()); + + if (_hasAnyCascadingParameterSubscriptions) + { + RemoveCascadingParameterSubscriptions(); + } + return DisposeAsync(); }