BrowserViewportService should implement IDisposable

[Kumima]

Bug type

Component

Component name

BrowserViewportService

What happened?

Related #3741 When using new blazor web template, we use app.UseExceptionHandler("/Error", createScopeForErrors: true);. When a new service scope is created, the old ones are disposed by framework, which results:

System.InvalidOperationException: 'MudBlazor.BrowserViewportService' type only implements IAsyncDisposable. Use DisposeAsync to dispose the container. at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.Dispose() at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddlewareImpl.HandleException(HttpContext context, ExceptionDispatchInfo edi)

I did not investigate too much, maybe the dispose process is different between this and circuit close? I did not notice exception for the other scenarios.

Expected behavior

Implement IDisposable for BrowserViewportService.

Reproduction link

https://github.com/MudBlazor/MudBlazor/issues/3741

Reproduction steps

1. Create a new project of Blazor Web with default template.
2. Throw exception in OnInitialized() of Home page
3. Check console

Relevant log output

No response

Version (bug)

Latest

Version (working)

No response

What browsers are you seeing the problem on?

Microsoft Edge

On what operating system are you experiencing the issue?

Windows

Pull Request

• [ ] I would like to do a Pull Request

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[yugabe]

This happens regardless of Blazor Server, as it also happens during Blazor SSR (without SignalR).

Using the error handler, specifically, seems to dispose the previous ServiceProvider, because it needs to re-execute the pipeline for another URL. This explains why the framework needs to dispose of and recreate the ServiceProvider scope. It's quite hard to work around the issue as well, because you need to implement a fake on the server for yourself:

internal sealed class MudFakeServerService : IBrowserViewportService, IBreakpointService, IPopoverService, IMudPopoverService, IDisposable
{
    // implement all interface members as noop
}

// later on:
builder.Services.AddSingleton<IBrowserViewportService, MudFakeServerService>();
builder.Services.AddSingleton<IBreakpointService, MudFakeServerService>();
builder.Services.AddSingleton<IPopoverService, MudFakeServerService>();
builder.Services.AddSingleton<IMudPopoverService, MudFakeServerService>();

Yes, you need to implement it with all of the services listed above, because even though the BrowserViewportService throws the first-time, the others follow in suit.

The fix would be quite simple on the framework's side I believe, because all IAsyncDisposable implementations I came across are either sync already or do fire-and-forget async, although I haven't investigated thoroughly.

I believe @ScarletKuro would be able to elaborate. I believe this would be an easily fixable issue.

[changingmission]

Still not resolved?

[ScarletKuro]

It appears to be a bug in the Microsoft code to me. Why would it only call Dispose to dispose the container? I went to aspnetcore repository to check the ExceptionHandlerMiddlewareImpl implementation and was about to fill a bug report when I discovered that an fix was merged one week ago (https://github.com/dotnet/aspnetcore/pull/52687). However, it seems like this fix will only be shipped in .NET 9 =/

I'm aware of the recommendation provided here (https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-disposeasync), but there are scenarios where a class implementing IAsyncDisposable cannot implement IDisposable. This is our cases and they are totally valid and should not cause any problems. The problem is that WASM doesn't support GetAwaiter().GetResult() (which would immediately deadlock). So I'm not really sure how to solve it, do we just use fire and forget or create an empty Dispose method? I find this as workarounds and they can create it's own problems.

It's worth mentioning that Microsoft also has classes that only implement IAsyncDisposable, and there are a lot of cases when it does implement both a simple GetAwaiter().GetResult() is used in the Dispose and when it doesn't it just lucky to have sync and async versions of API.

The fix would be quite simple on the framework's side I believe, because all IAsyncDisposable implementations I came across are either sync already or do fire-and-forget async, although I haven't investigated thoroughly.

Yes, some are like BrowserViewportService can and yet they are only because of another bug in MS code base, the js interop is called in fire and forget manner, but for example it would hard to implement Dispose in IPopoverService.

[yugabe]

I see, I didn't investigate why it wasn't disposing of IAsyncDisposable services correctly.

I think it is safe to assume though that services that don't await anything in DisposeAsync can safely be implemented by

void IDisposable Dispose() => DisposeAsync();

This at least merits a comment in source, however.

For others that do, like PopoverService, it's more problematic. In my case specifically, this happened during an error was trying to re-execute the pipeline, so there "shouldn't" be any async work happening because of JS interop, as the browser wasn't even available (server was still prerendering). Further, this happens during SSR as well, where there is no JS interop to speak of whatsoever. There might be other async work theoretically that is not JS interop, but I'm not sure there is in MudBlazor's case.

I haven't really thought it all the way through, but maybe it would be worth considering having different service implementations for server side or client side for where it makes sense? In my case specifically, which won't be a match for everyone, the server side components should simply be noop, or at least, non-interactive, because I don't use Blazor Server in the app that produced this error. It might actually be a different issue to tackle that, but I believe it's worth considering now how you plan to integrate this along the way. I think in .NET 9 there will be a way to check the current render mode from code, which can theoretically solve a few issues, but I'm not sure work even started on that already.

[ScarletKuro]

this happened during an error was trying to re-execute the pipeline, so there "shouldn't" be any async work happening because of JS interop, as the browser wasn't even available (server was still prerendering)

There is no API to identify what rendering mode is used. Also there is no API to understand if JS runtime is available, you can understand it in a razor component after OnAfterRender but not in a service, there is only a pesky workaround available to check if the JsRuntime implements UnsupportedJavaScriptRuntime

maybe it would be worth considering having different service implementations for server side or client side for where it makes sense?

I'm not sure about that, would need to talk with team, it would mean that there is no single AddMudServices for both projects and we would need to have specific extension for server and client side, which would complicate things a little for end users.

[ScarletKuro]

Can someone actually post a minimalistic reproduction, instead to redirect to an issue that redirects to our code in the "Reproduction link" section =/

Reproduction steps

1. Create a new project of Blazor Web with default template.
2. Throw exception in `OnInitialized()` of Home page
3. Check console

I tied to do this steps with the app.UseExceptionHandler("/Error", createScopeForErrors: true) line in the debug mode, but I do not see the mentioned exception:

type only implements IAsyncDisposable. Use DisposeAsync to dispose the container.

Not in browser console, not in the service console, not in error page. I only see the exception trace that was thrown in the OnInitialized. I also tried release mode and ASPNETCORE_ENVIRONMENT to production, but no. What I'm doing wrongl? I just want to send this to Microsoft so they would port the fix.

[yugabe]

I would, but I don't have access to a dev box for quite a few days, at least. You wouldn't see this happening in the browser, it only pertains to the server's service provider. From memory, all I can say is, make sure you try to use some of the affected MudBlazor services (directly or indirectly) in both the page that throws AND the Error page as well (might be both or just one or the other that causes the issue).

[ScarletKuro]

make sure you try to use some of the affected MudBlazor services (directly or indirectly) in both the page that throws AND the Error page as well (might be both or just one or the other that causes the issue).

I created a clean Server Side WebApp application, without MudBlazor (it should be reproducible without MudBlazor for obvious reasons). Created a type that implements IAsyncDisposable only and added it as scoped to the DI. Injected it in Home page, and throw error in OnInitialized. I see that IAsyncDisposable is called, and the error page is shown, but the error IAsyncDisposable is nowhere to observe.

[yugabe]

That sounds suspiciously close to the underlying bug being fixed and backported to .NET 8. Someone will have to confirm if and how they're able to repro at this point.

[ScarletKuro]

That sounds suspiciously close to the underlying bug being fixed and backported to .NET 8. Someone will have to confirm if and how they're able to repro at this point.

I thought so too, but when navigating to ExceptionHandlerMiddleware and then ExceptionHandlerMiddlewareImpl I see the old code, at least this is what SourceLink is telling me.

[ScarletKuro]

Hi, I’m closing this issue since net9 will release in November this year, which includes a fix for the exception middleware. We will not implement IDisposable in our services. According to Microsoft’s documentation, implementing IDisposable is only required when disposal is handled by the customer or when a class contains instances of both implementations. In our case, neither of these conditions applies, as disposal is managed by DI and only involves asynchronous resources.