Skip to content
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

Blazor doesn't DisposeAsync DI scope #12918

Closed
analogrelay opened this issue Aug 6, 2019 · 9 comments
Closed

Blazor doesn't DisposeAsync DI scope #12918

analogrelay opened this issue Aug 6, 2019 · 9 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed

Comments

@analogrelay
Copy link
Contributor

Blazor doesn't call DisposeAsync on the DI scope, which leads to the following exception being logged during Circuit disposal:

fail: Microsoft.AspNetCore.Components.Server.Circuits.CircuitRegistry[100]
      Unhandled exception disposing circuit host: 'blazor_async_dispose.MyScopedDisposableService' type only implements IAsyncDisposable. Use DisposeAsync to dispose the container.
System.InvalidOperationException: 'blazor_async_dispose.MyScopedDisposableService' type only implements IAsyncDisposable. Use DisposeAsync to dispose the container.
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.Dispose()
   at Microsoft.AspNetCore.Components.Server.Circuits.CircuitHost.<DisposeAsync>b__51_0()
   at Microsoft.AspNetCore.Components.Rendering.RendererSynchronizationContext.<>c.<<InvokeAsync>b__9_0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AspNetCore.Components.Server.Circuits.CircuitHost.DisposeAsync()
   at Microsoft.AspNetCore.Components.Server.Circuits.CircuitRegistry.DisposeCircuitEntry(DisconnectedCircuitEntry entry)

The DI scope/container is rigid about which kind of disposal you use depending on the services you have. This matrix lays out what happens. Across the top are the kinds of services you have, down the left is how you dispose the scope:

How you dispose the scope Implement IDisposable only Implement both IAsyncDisposable and IDisposable Implement IAsyncDisposable only
DisposeAsync OK (calls Dispose) OK (calls DisposeAsync) OK (calls DisposeAsync)
Dispose OK (calls Dispose) OK (calls Dispose) ❌Throws❌

It's definitely arguable that throwing this exception in DI isn't appropriate. I just want to open the discussion somewhere.

Repro Steps

  1. dotnet new blazorserver
  2. Add file MyScopedDisposableService.cs:
public class MyScopedDisposableService : IAsyncDisposable
{
    public MyScopedDisposableService()
    {
        Console.WriteLine("Scoped Service Constructed");
    }

    public ValueTask DisposeAsync()
    {
        Console.WriteLine("Scoped Service Disposed async!");
        return default;
    }
}
  1. Register in ConfigureServices:
services.AddScoped<MyScopedDisposableService>();
  1. Inject into Pages/Index.razor:
@inject MyScopedDisposableService Service
  1. Launch the app.
  2. Navigate to the index page.
  3. Close the tab.
  4. Wait about ~5 minutes for the circuit to get disposed.

Expected Behavior

No error is logged

Actual Behavior

The following error is logged:

fail: Microsoft.AspNetCore.Components.Server.Circuits.CircuitRegistry[100]
      Unhandled exception disposing circuit host: 'blazor_async_dispose.MyScopedDisposableService' type only implements IAsyncDisposable. Use DisposeAsync to dispose the container.
System.InvalidOperationException: 'blazor_async_dispose.MyScopedDisposableService' type only implements IAsyncDisposable. Use DisposeAsync to dispose the container.
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.Dispose()
   at Microsoft.AspNetCore.Components.Server.Circuits.CircuitHost.<DisposeAsync>b__51_0()
   at Microsoft.AspNetCore.Components.Rendering.RendererSynchronizationContext.<>c.<<InvokeAsync>b__9_0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AspNetCore.Components.Server.Circuits.CircuitHost.DisposeAsync()
   at Microsoft.AspNetCore.Components.Server.Circuits.CircuitRegistry.DisposeCircuitEntry(DisconnectedCircuitEntry entry)
@analogrelay analogrelay added the area-blazor Includes: Blazor, Razor Components label Aug 6, 2019
@analogrelay
Copy link
Contributor Author

analogrelay commented Aug 6, 2019

If we do fix this in Blazor instead of DI, we also need to look at other places we create scopes and change to dispose them async.

@analogrelay
Copy link
Contributor Author

analogrelay commented Aug 6, 2019

My initial feeling is that throwing in Dispose is bad and DI shouldn't do it (for example: https://docs.microsoft.com/en-us/visualstudio/code-quality/ca1065-do-not-raise-exceptions-in-unexpected-locations?view=vs-2019)

@analogrelay
Copy link
Contributor Author

@davidfowl
Copy link
Member

As part of our sync over async awareness, we need to throw by default and have switches for not throwing.

@pranavkm
Copy link
Contributor

pranavkm commented Aug 6, 2019

What about things that aren't in DI scope? For instance, Controller or Hub instances: https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/Mvc.Core/src/Controllers/DefaultControllerActivator.cs#L72-L75

@davidfowl
Copy link
Member

Should have been updated during the 3.x milestone but we missed those places.

@mkArtakMSFT mkArtakMSFT added the bug This issue describes a behavior which is not expected - a bug. label Aug 7, 2019
@mkArtakMSFT mkArtakMSFT added this to the 3.0.0-preview9 milestone Aug 7, 2019
rynowak added a commit that referenced this issue Aug 14, 2019
Fixes part of #12918

This fixes the part of this issue that we're going to be able to do in
3.0 safely.
rynowak added a commit that referenced this issue Aug 14, 2019
Fixes part of #12918

This fixes the part of this issue that we're going to be able to do in
3.0 safely.
@rynowak
Copy link
Member

rynowak commented Aug 14, 2019

I've made a change for 3.0 to support disposing the scope created by CircuitHost asynchronously. We still need to do work in Blazor to support async disposal of components - tracked by #9960

@rynowak rynowak closed this as completed Aug 14, 2019
@rynowak rynowak added the Done This issue has been fixed label Aug 14, 2019
@davidfowl
Copy link
Member

Do we have something tracking mvc as well?

@rynowak
Copy link
Member

rynowak commented Aug 14, 2019

I just #13150 created for it.

I don't think it's really time-sensitive that we have this in MVC, it's more about completeness.

  • The components programming model is stateful, so you frequently write disposable components
  • We don't really encourage writing disposable controllers in MVC (supported for completeness and legacy). Relying on DI for disposal us usually easier and better.
  • Blazor creates scopes (server side)
  • MVC doesn't create scopes

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

5 participants