Skip to content

OwningComponentBase implements IAsyncDisposable #62583

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
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
55 changes: 49 additions & 6 deletions src/Components/Components/src/OwningComponentBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace Microsoft.AspNetCore.Components;
/// requires disposal such as a repository or database abstraction. Using <see cref="OwningComponentBase"/>
/// as a base class ensures that the service provider scope is disposed with the component.
/// </remarks>
public abstract class OwningComponentBase : ComponentBase, IDisposable
public abstract class OwningComponentBase : ComponentBase, IDisposable, IAsyncDisposable
{
private AsyncServiceScope? _scope;

Expand Down Expand Up @@ -44,20 +44,63 @@ protected IServiceProvider ScopedServices
}
}

/// <summary>
/// Releases the service scope used by the component.
/// </summary>
Comment on lines +47 to +49
Copy link
Member

Choose a reason for hiding this comment

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

I sense the AI working here 😄

These methods are not public, so no need to document it. In .NET when a method is an interface implementation, it can use /// <inhertidoc /> to tell it to use the same text as the interface definition.

void IDisposable.Dispose()
{
Dispose(disposing: true);
GC.SuppressFinalize(this);
}

/// <summary>
/// Releases the service scope used by the component.
/// </summary>
/// <param name="disposing">true to release both managed and unmanaged resources; false to release only unmanaged resources.</param>
protected virtual void Dispose(bool disposing)
{
if (!IsDisposed)
{
_scope?.Dispose();
_scope = null;
Dispose(disposing: true);
if (disposing)
{
if (_scope.HasValue)
{
if (_scope.Value is IDisposable disposable)
{
disposable.Dispose();
}
_scope = null;
}
}
Comment on lines +64 to +74
Copy link
Member

Choose a reason for hiding this comment

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

You can probably combine the three conditions rather than nesting them:
if(disposing && _scope.HasValue && _scope.Value is IDisposable disposable)

Might be possible to also just do
if(disposing && _scope.Value is IDisposable disposable) (without the has value check). You can set _scope = null unconditionally inside the clause

IsDisposed = true;
}
}

/// <inheritdoc />
protected virtual void Dispose(bool disposing)
/// <summary>
/// Asynchronously releases the service scope used by the component.
/// </summary>
/// <returns>A task that represents the asynchronous dispose operation.</returns>
async ValueTask IAsyncDisposable.DisposeAsync()
{
await DisposeAsyncCore().ConfigureAwait(false);

Dispose(disposing: false);
GC.SuppressFinalize(this);
}

/// <summary>
/// Asynchronously releases the service scope used by the component.
/// </summary>
/// <returns>A task that represents the asynchronous dispose operation.</returns>
protected virtual async ValueTask DisposeAsyncCore()
Copy link
Member

Choose a reason for hiding this comment

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

Note for API review. Since this is a "standard" implementation pattern that people might have followed within their own classes, it might be problematic if we use the same name (as the name will start conflicting) so we should consider "breaking away" from this and using a different name to "avoid the breackage".

Overall, I think this is potentially uncommon since OwningComponentBase is not common in the first place.

{
if (!IsDisposed && _scope.HasValue)
{
await _scope.Value.DisposeAsync().ConfigureAwait(false);
_scope = null;
}

IsDisposed = true;
}
}

Expand Down
1 change: 1 addition & 0 deletions src/Components/Components/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@ Microsoft.AspNetCore.Components.Infrastructure.PersistentStateProviderServiceCol
static Microsoft.AspNetCore.Components.Infrastructure.RegisterPersistentComponentStateServiceCollectionExtensions.AddPersistentServiceRegistration<TService>(Microsoft.Extensions.DependencyInjection.IServiceCollection! services, Microsoft.AspNetCore.Components.IComponentRenderMode! componentRenderMode) -> Microsoft.Extensions.DependencyInjection.IServiceCollection!
static Microsoft.AspNetCore.Components.Infrastructure.ComponentsMetricsServiceCollectionExtensions.AddComponentsMetrics(Microsoft.Extensions.DependencyInjection.IServiceCollection! services) -> Microsoft.Extensions.DependencyInjection.IServiceCollection!
static Microsoft.AspNetCore.Components.Infrastructure.ComponentsMetricsServiceCollectionExtensions.AddComponentsTracing(Microsoft.Extensions.DependencyInjection.IServiceCollection! services) -> Microsoft.Extensions.DependencyInjection.IServiceCollection!
virtual Microsoft.AspNetCore.Components.OwningComponentBase.DisposeAsyncCore() -> System.Threading.Tasks.ValueTask
static Microsoft.AspNetCore.Components.Infrastructure.PersistentStateProviderServiceCollectionExtensions.AddSupplyValueFromPersistentComponentStateProvider(this Microsoft.Extensions.DependencyInjection.IServiceCollection! services) -> Microsoft.Extensions.DependencyInjection.IServiceCollection!
virtual Microsoft.AspNetCore.Components.Rendering.ComponentState.GetComponentKey() -> object?
66 changes: 66 additions & 0 deletions src/Components/Components/test/OwningComponentBaseTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,69 @@ public void CreatesScopeAndService()
Assert.Equal(1, counter.DisposedCount);
}

[Fact]
public async Task DisposeAsyncReleasesScopeAndService()
{
var services = new ServiceCollection();
services.AddSingleton<Counter>();
services.AddTransient<MyService>();
var serviceProvider = services.BuildServiceProvider();

var counter = serviceProvider.GetRequiredService<Counter>();
var renderer = new TestRenderer(serviceProvider);
var component1 = (MyOwningComponent)renderer.InstantiateComponent<MyOwningComponent>();

Assert.NotNull(component1.MyService);
Assert.Equal(1, counter.CreatedCount);
Assert.Equal(0, counter.DisposedCount);
Assert.False(component1.IsDisposedPublic);

await ((IAsyncDisposable)component1).DisposeAsync();
Assert.Equal(1, counter.CreatedCount);
Assert.Equal(1, counter.DisposedCount);
Assert.True(component1.IsDisposedPublic);
}

[Fact]
public void ThrowsWhenAccessingScopedServicesAfterDispose()
{
var services = new ServiceCollection();
services.AddSingleton<Counter>();
services.AddTransient<MyService>();
var serviceProvider = services.BuildServiceProvider();

var renderer = new TestRenderer(serviceProvider);
var component1 = (MyOwningComponent)renderer.InstantiateComponent<MyOwningComponent>();

// Access service first to create scope
var service = component1.MyService;

((IDisposable)component1).Dispose();

// Should throw when trying to access services after disposal
Assert.Throws<ObjectDisposedException>(() => component1.MyService);
}

[Fact]
public async Task ThrowsWhenAccessingScopedServicesAfterDisposeAsync()
{
var services = new ServiceCollection();
services.AddSingleton<Counter>();
services.AddTransient<MyService>();
var serviceProvider = services.BuildServiceProvider();

var renderer = new TestRenderer(serviceProvider);
var component1 = (MyOwningComponent)renderer.InstantiateComponent<MyOwningComponent>();

// Access service first to create scope
var service = component1.MyService;

await ((IAsyncDisposable)component1).DisposeAsync();

// Should throw when trying to access services after disposal
Assert.Throws<ObjectDisposedException>(() => component1.MyService);
}

private class Counter
{
public int CreatedCount { get; set; }
Expand All @@ -51,5 +114,8 @@ public MyService(Counter counter)
private class MyOwningComponent : OwningComponentBase<MyService>
{
public MyService MyService => Service;

// Expose IsDisposed for testing
public bool IsDisposedPublic => IsDisposed;
}
}
Loading