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

MauiApp is not Disposed #20350

Open
malciin opened this issue Feb 4, 2024 · 11 comments
Open

MauiApp is not Disposed #20350

malciin opened this issue Feb 4, 2024 · 11 comments
Labels
area-core-hosting Extensions / Hosting / AppBuilder / Startup platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working

Comments

@malciin
Copy link

malciin commented Feb 4, 2024

Description

I have noticed that all of my Singleton services are not properly disposed in my app. Upon further investigation I've noticed thats probably (I'm newbie in maui) because neither of:

public void Dispose()

public async ValueTask DisposeAsync()

was called.

Steps to Reproduce

  1. Create singleton service that implements IDisposable
  2. Set breakpoint in dispose code.
  3. Close window by clicking X

Results: Dispose never gets called thus breakpoint never occur.

Reproduction repo: malciin/maui-blazor-dispose-issue@e031693

Link to public reproduction project repository

malciin/maui-blazor-dispose-issue@e031693

Version with bug

8.0.3

Is this a regression from previous behavior?

Not sure, did not test other versions

Last version that worked well

Unknown/Other

Affected platforms

Windows, I was not able test on other platforms

Affected platform versions

No response

Did you find any workaround?

No response

Relevant log output

No response

@malciin malciin added the t/bug Something isn't working label Feb 4, 2024
@malciin malciin changed the title MauiApp not Disposed in maui-blazor MauiApp is not Disposed in maui-blazor Feb 4, 2024
@jsuarezruiz jsuarezruiz added area-blazor Blazor Hybrid / Desktop, BlazorWebView area-perf Startup / Runtime performance labels Feb 5, 2024
@drasticactions
Copy link
Contributor

drasticactions commented Feb 6, 2024

https://github.com/drasticactions/MauiRepros/tree/main/DisposeTest

Doing this in a regular MAUI app causes the same behavior to occur. MauiApp is disposable, but nothing I can find so far shows that anything invokes it to be disposed of upon app close. If anything, MauiApp is created and its references are used to build MauiContext, but MauiApp itself isn't attached to anything once it's been called.

Now, that seems kinda weird to me (Why make the object just to create a new thing off of it and throw it away) but as for Dispose itself, I don't think this is a bug. To me, that makes sense: Your app is closing. The resources you were using are going to be released anyway. Calling disposed doesn't make sense there. And if you were doing logic within your app in its Disposed methods to handle when the app is closing... I would rethink that approach.

If you were using MAUI in an embedded app case, where you create MauiApp yourself and control its existence, being able to dispose it at will makes sense, as you may only need it for a specific part of your app. But for the general case of a MAUI or MAUI Blazor app, you would never dispose MauiApp yourself as that would break your app altogether, and disposing it when the app is closed, to me, doesn't make sense as your app is closing and those resources are going to be released anyway.

@jonathanpeppers @mattleibow What do ya'll think?

@malciin
Copy link
Author

malciin commented Feb 6, 2024

@drasticactions I have to say that your answer is pretty... confusing/surprising at least.

that seems kinda weird to me (Why make the object just to create a new thing off of it and throw it away)

I thought it was obvious this is a minimal reproduction code that does not doing anything meaningful - just reproducing the error. I've added singleton service that needs to be disposed and it is not.

And if you were doing logic within your app in its Disposed methods to handle when the app is closing... I would rethink that approach.

Really? Disposing object needs rethinking? I can agree that I shouldn't put long running code in disposal but not with that. Dispose can do many useful things, some singleton services could spawn some Process and then in Dispose() could gracefully stopped them. I am little baffled that I need to explain that.

In asp net core or in console app that uses Microsoft.Extensions.Hosting everything is correctly disposed at graceful app stop, so I am really surprised that maui blazor framework not disposing MauiApp (that implements IDisposable) is not a bug and I should rethink my app design.

@MartyIX
Copy link
Collaborator

MartyIX commented Feb 6, 2024

[..] And if you were doing logic within your app in its Disposed methods to handle when the app is closing... I would rethink that approach.

We do and we miss graceful/orderly shutdown of our MAUI app very much. Basically user clicks "X" button and we would like to have an opportunity to dispose stuff.

There is this PR #13424 but it seems like there is no good moment for us to call await app.DisposeAsync(). We tried quite hard and we could somehow managed to use app.Dispose() and it seems to work for us but it's hackish as MAUI does not support this scenario that well (or I don't know how to make it work properly).

We would appreciate to terminate a TCP connection to another APP, some websockets, etc. This is IMO a valid intention, even if it is not 100 per cent successful in practice (everybody knows that).

Honestly, I don't see this "cannot easily dispose" as a feature.

@drasticactions
Copy link
Contributor

drasticactions commented Feb 6, 2024

@drasticactions I have to say that your answer is pretty... confusing/surprising at least.

that seems kinda weird to me (Why make the object just to create a new thing off of it and throw it away)

I thought it was obvious this is a minimal reproduction code that does not doing anything meaningful - just reproducing the error. I've added singleton service that needs to be disposed and it is not.

I'm sorry! I wasn't talking about your example, I was talking about the MAUI code (As in, code in this repo)!

var mauiApp = CreateMauiApp();
var rootContext = new MauiContext(mauiApp.Services);
var applicationContext = rootContext.MakeApplicationScope(this);

This specifically creates a MauiApp object, but then doesn't store it anywhere. Even if you wanted to dispose it, you couldn't unless you went out of your way to do so (By storing the MauiApp when it's created within the individual platform code). I wasn't talking about your sample, what you did was 100% fine. I should have been more clear.

@mattleibow mattleibow changed the title MauiApp is not Disposed in maui-blazor MauiApp is not Disposed Feb 6, 2024
@Eilon
Copy link
Member

Eilon commented Feb 7, 2024

Indeed, in normal circumstances app termination is 'violent' - that is, it is both sudden and catastrophic. Very little is cleaned up because the idea is to quickly make the app go away. If anything was truly important, it should have already been done (data was saved, messages sent, etc.).

So why does MauiApp bother with IDisposable? For testing purposes only, really. I mean, you could probably cause it to be disposed with some extra code in your app, so long as you're aware that it still might not get called (process can be easily terminated without any code in the target process being run).

@Eilon Eilon added area-core-hosting Extensions / Hosting / AppBuilder / Startup and removed area-blazor Blazor Hybrid / Desktop, BlazorWebView area-perf Startup / Runtime performance labels Feb 7, 2024
@Eilon
Copy link
Member

Eilon commented Feb 7, 2024

Here's a related PR where I added IAsyncDisposable support, but also only for testing purposes: #13424

@malciin
Copy link
Author

malciin commented Feb 7, 2024

Indeed, in normal circumstances app termination is 'violent' - that is, it is both sudden and catastrophic.

@Eilon I have to disagree - especially in terms of maui-blazor for windows and also generally for windows gui desktop apps. For GUI desktop apps in general - the "violent termination" is not a normal circumstance - its an exceptional case.

For example in most apps when you click X you can have question about unsaved changes etc, other apps becomes unrensponsible for around 100-150ms because after clicking X they performs additional "cleanup steps" like flushing internal buffers etc, so I am surprised why clicking X in MAUI app is treated as "violent sudden and catastrophic".

Maybe for IOS/Android thats the requirements and that systems forces app to quit immediately, but if MAUI app targets windows platforms then I guess Disposing MauiApp would be really beneficial.

@Eilon
Copy link
Member

Eilon commented Feb 7, 2024

@malciin I think that the scenarios you describe are valid, but I'm not sure they're part of "disposing the MauiApp object". If an app wants to ask the user whether to save/upload/re-combobulate when they try to close the app, then that specific scenario should be handled as part of the "closing the window" event (whatever it might be called).

@malciin
Copy link
Author

malciin commented Feb 7, 2024

(..) then that specific scenario should be handled as part of the "closing the window" event (whatever it might be called).

To me thats what IDisposable are for. To me thats also what DI simplifies - dispose any created class instances that need disposal. If I register singleton service with IDisposable I, as a programmer, expect that to be disposed. Maybe asking user for save was a bad example, but flushing internal buffers, gracefuly closing tcp connections etc are those examples that I would put in Dispose() and expect them to happend. X should be graceful shutdown, not "violent, sudden and catastrophic".
Many logging libraries writes to files at specific intervals. If MauiApp is not disposed that could ment that some logs could be lost.

Lets put it differently. Why Console app that uses Microsoft.Extensions.Hosting can support graceful shutdown, every singleton instance is nicely disposed in that case, buffers are flushed blah blah, but maui windows app can not do that? What are the differences between them (for windows platform) that make "maui not disposing - good", "console app not disposing - bad"? Why maui app can not dispose itself in main window close event?

If anything was truly important, it should have already been done (data was saved, messages sent, etc.).

So lets make Microsoft.Extension.Hosting to just close without disposal at all. If anything was truly important it should have already been done :) "To me, that makes sense: App is closing. The resources that app were using are going to be released anyway. Calling disposed doesn't make sense there."

@Eilon
Copy link
Member

Eilon commented Mar 1, 2024

I've given this some more thought and it could be worth considering. For anyone interested in this, I have two questions:

  1. Under what "app lifecycle event" would you expect the MauiApp to be disposed? (Is there already such a cross-platform MAUI event? Or is there at least one on each platform?)
  2. What should we do for IAsyncDisposable? Generally the app lifecycle events are sync only, and there's no safe way to process IAsyncDisposables (we tried something similar in BlazorWebView and it's caused a ton of problems - problems that we don't even know how to fix). Should it do async dispose, but only give it up to let's say 500ms and then terminate? Or some other pattern you can think of?

I think if we can come up with reasonable answers for these then it's worth giving it a shot.

@ninachen03 ninachen03 added s/verified Verified / Reproducible Issue ready for Engineering Triage s/triaged Issue has been reviewed labels Apr 28, 2024
@ninachen03
Copy link
Collaborator

Verified this issue with Visual Studio 17.10.0 Preview 5 ( 8.0.21 &8.0.0-rc.2.9530).I can repro this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-hosting Extensions / Hosting / AppBuilder / Startup platform/windows 🪟 s/triaged Issue has been reviewed s/verified Verified / Reproducible Issue ready for Engineering Triage t/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants