Skip to content

Dispose components on client disconnects #6693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/Components/.editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,31 @@ indent_size = 2

[*.{xml,csproj,config,*proj,targets,props}]
indent_size = 2

# Dotnet code style settings:
[*.cs]
# Sort using and Import directives with System.* appearing first
dotnet_sort_system_directives_first = true

# Don't use this. qualifier
dotnet_style_qualification_for_field = false:suggestion
dotnet_style_qualification_for_property = false:suggestion

# use int x = .. over Int32
dotnet_style_predefined_type_for_locals_parameters_members = true:suggestion

# use int.MaxValue over Int32.MaxValue
dotnet_style_predefined_type_for_member_access = true:suggestion

# Require var all the time.
csharp_style_var_for_built_in_types = true:suggestion
csharp_style_var_when_type_is_apparent = true:suggestion
csharp_style_var_elsewhere = true:suggestion

# Newline settings
csharp_new_line_before_open_brace = all
csharp_new_line_before_else = true
csharp_new_line_before_catch = true
csharp_new_line_before_finally = true
csharp_new_line_before_members_in_object_initializers = true
csharp_new_line_before_members_in_anonymous_types = true
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
/// Provides mechanisms for rendering <see cref="IComponent"/> instances in a
/// web browser, dispatching events to them, and refreshing the UI as required.
/// </summary>
public class WebAssemblyRenderer : Renderer, IDisposable
public class WebAssemblyRenderer : Renderer
{
private readonly int _webAssemblyRendererId;

Expand Down Expand Up @@ -71,11 +71,10 @@ public void AddComponent(Type componentType, string domElementSelector)
RenderRootComponent(componentId);
}

/// <summary>
/// Disposes the instance.
/// </summary>
public void Dispose()
/// <inheritdoc />
protected override void Dispose(bool disposing)
{
base.Dispose(disposing);
RendererRegistry.Current.TryRemove(_webAssemblyRendererId);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@

[assembly: InternalsVisibleTo("Microsoft.AspNetCore.Blazor, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
[assembly: InternalsVisibleTo("Microsoft.AspNetCore.Components.Server, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]

[assembly: InternalsVisibleTo("Microsoft.AspNetCore.Components.Server.Test, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
Original file line number Diff line number Diff line change
Expand Up @@ -16,31 +16,26 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits
{
internal class CircuitHost : IDisposable
{
private static AsyncLocal<CircuitHost> _current = new AsyncLocal<CircuitHost>();
private static readonly AsyncLocal<CircuitHost> _current = new AsyncLocal<CircuitHost>();

/// <summary>
/// Gets the current <see cref="Circuit"/>, if any.
/// </summary>
public static CircuitHost Current => _current.Value;

/// <summary>
/// Sets the current <see cref="Circuit"/>.
/// Sets the current <see cref="Circuits.Circuit"/>.
/// </summary>
/// <param name="circuitHost">The <see cref="Circuit"/>.</param>
/// <param name="circuitHost">The <see cref="Circuits.Circuit"/>.</param>
/// <remarks>
/// Calling <see cref="SetCurrentCircuitHost(CircuitHost)"/> will store the circuit
/// and other related values such as the <see cref="IJSRuntime"/> and <see cref="Renderer"/>
/// in the local execution context. Application code should not need to call this method,
/// it is primarily used by the Server-Side Blazor infrastructure.
/// it is primarily used by the Server-Side Components infrastructure.
/// </remarks>
public static void SetCurrentCircuitHost(CircuitHost circuitHost)
{
if (circuitHost == null)
{
throw new ArgumentNullException(nameof(circuitHost));
}

_current.Value = circuitHost;
_current.Value = circuitHost ?? throw new ArgumentNullException(nameof(circuitHost));

Microsoft.JSInterop.JSRuntime.SetCurrentJSRuntime(circuitHost.JSRuntime);
RendererRegistry.SetCurrentRendererRegistry(circuitHost.RendererRegistry);
Expand Down Expand Up @@ -134,6 +129,7 @@ await SynchronizationContext.Invoke(() =>
public void Dispose()
{
Scope.Dispose();
Renderer.Dispose();
}

private void AssertInitialized()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,10 @@ public override Task InvokeAsync(Func<Task> workItem)
}
}

/// <summary>
/// Disposes the instance.
/// </summary>
public void Dispose()
/// <inheritdoc />
protected override void Dispose(bool disposing)
{
base.Dispose(true);
_rendererRegistry.TryRemove(_id);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,16 @@ namespace Microsoft.AspNetCore.Components.Rendering
/// Provides mechanisms for rendering hierarchies of <see cref="IComponent"/> instances,
/// dispatching events to them, and notifying when the user interface is being updated.
/// </summary>
public abstract class Renderer
public abstract class Renderer : IDisposable
{
private readonly ComponentFactory _componentFactory;
private int _nextComponentId = 0; // TODO: change to 'long' when Mono .NET->JS interop supports it
private readonly Dictionary<int, ComponentState> _componentStateById
= new Dictionary<int, ComponentState>();

private readonly Dictionary<int, ComponentState> _componentStateById = new Dictionary<int, ComponentState>();
private readonly RenderBatchBuilder _batchBuilder = new RenderBatchBuilder();
private bool _isBatchInProgress;
private readonly Dictionary<int, EventHandlerInvoker> _eventBindings = new Dictionary<int, EventHandlerInvoker>();

private int _nextComponentId = 0; // TODO: change to 'long' when Mono .NET->JS interop supports it
private bool _isBatchInProgress;
private int _lastEventHandlerId = 0;
private readonly Dictionary<int, EventHandlerInvoker> _eventBindings = new Dictionary<int, EventHandlerInvoker>();

/// <summary>
/// Constructs an instance of <see cref="Renderer"/>.
Expand Down Expand Up @@ -175,7 +173,7 @@ internal void AssignEventHandlerId(ref RenderTreeFrame frame)

if (frame.AttributeValue is MulticastDelegate @delegate)
{
_eventBindings.Add(id, new EventHandlerInvoker(@delegate));
_eventBindings.Add(id, new EventHandlerInvoker(@delegate));
}

frame = frame.WithAttributeEventHandlerId(id);
Expand Down Expand Up @@ -295,5 +293,44 @@ private void RemoveEventHandlerIds(ArrayRange<int> eventHandlerIds, Task afterTa
RemoveEventHandlerIds(eventHandlerIdsClone, Task.CompletedTask));
}
}

/// <summary>
/// Releases all resources currently used by this <see cref="Renderer"/> instance.
/// </summary>
/// <param name="disposing"><see langword="true"/> if this method is being invoked by <see cref="IDisposable.Dispose"/>, otherwise <see langword="false"/>.</param>
protected virtual void Dispose(bool disposing)
{
List<Exception> exceptions = null;

foreach (var componentState in _componentStateById.Values)
{
if (componentState.Component is IDisposable disposable)
{
try
{
disposable.Dispose();
}
catch (Exception exception)
{
// Capture exceptions thrown by individual components and rethrow as an aggregate.
exceptions = exceptions ?? new List<Exception>();
exceptions.Add(exception);
}
}
}

if (exceptions != null)
{
throw new AggregateException(exceptions);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have an issue somewhere to clean this up? Throwing inside dispose isn't a nice thing to do, especially when you have inheritance involved. There are a bunch of places in this PR that will leak stuff on failed dispose 😆

I realize that you didn't make it worse in this PR, but it is gross so I want to make sure it's on someone's plate to fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it to this issue - #5476

}

/// <summary>
/// Releases all resources currently used by this <see cref="Renderer"/> instance.
/// </summary>
public void Dispose()
{
Dispose(disposing: true);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// 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 Microsoft.AspNetCore.Components.Browser;
using Microsoft.AspNetCore.Components.Browser.Rendering;
using Microsoft.AspNetCore.SignalR;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.JSInterop;
using Moq;
using Xunit;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort


namespace Microsoft.AspNetCore.Components.Server.Circuits
{
public class CircuitHostTest
{
[Fact]
public void Dispose_DisposesResources()
{
// Arrange
var serviceScope = new Mock<IServiceScope>();
var clientProxy = Mock.Of<IClientProxy>();
var renderRegistry = new RendererRegistry();
var jsRuntime = Mock.Of<IJSRuntime>();
var syncContext = new CircuitSynchronizationContext();

var remoteRenderer = new TestRemoteRenderer(
Mock.Of<IServiceProvider>(),
renderRegistry,
jsRuntime,
clientProxy,
syncContext);

var circuitHost = new CircuitHost(serviceScope.Object, clientProxy, renderRegistry, remoteRenderer, configure: _ => { }, jsRuntime: jsRuntime, synchronizationContext: syncContext);

// Act
circuitHost.Dispose();

// Assert
serviceScope.Verify(s => s.Dispose(), Times.Once());
Assert.True(remoteRenderer.Disposed);
}

private class TestRemoteRenderer : RemoteRenderer
{
public TestRemoteRenderer(IServiceProvider serviceProvider, RendererRegistry rendererRegistry, IJSRuntime jsRuntime, IClientProxy client, SynchronizationContext syncContext)
: base(serviceProvider, rendererRegistry, jsRuntime, client, syncContext)
{
}

public bool Disposed { get; set; }

protected override void Dispose(bool disposing)
{
base.Dispose(disposing);
Disposed = true;
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>netcoreapp3.0</TargetFramework>
Expand All @@ -7,6 +7,7 @@

<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.5.0" />
<PackageReference Include="Moq" Version="4.10.0" />
<PackageReference Include="xunit" Version="2.3.1" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.3.1" />
</ItemGroup>
Expand Down
Loading