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

Call Dispose() on Page and ViewModel when the page is popped if they implement IDisposable #7354

Closed
ederbond opened this issue May 20, 2022 · 90 comments

Comments

@ederbond
Copy link
Contributor

ederbond commented May 20, 2022

Description

It has been 2 years since I've opened this bug, and MSFT didn't come up with a proper solution, so to save your precious time consider completely ditching MAUI Shell for now and use the following library (based on PRISM v8) to workaround this issue:

https://github.com/naveasy/Naveasy

It would be amazing if MAUI framework could call the
Dispose method on the Page and also on it's ViewModel when the page gets removed from the navigation stack so we developers can gracefully dispose object that we have used and will no longer be used.
It would be even better if the same could be done on custom controls (Views) that we create.

Steps to Reproduce

Implement IDisposable on a given page;
Also Implement IDisposable on it's page's VM

Current behavior
The Dispose() method is never called after we remove the page from the navigation stack.

Expected behavior:
Dispose() to be called when the page is removed from the navigation stack;

Version with bug

Release Candidate 2 on .NET 8

Last version that worked well

Unknown/Other

Affected platforms

iOS, Android, Windows, macOS, Other (Tizen, Linux, etc. not supported by Microsoft directly)

Affected platform versions

Android 11 and iOS, Windows, Tizen, MacOS ...

Did you find any workaround?

No

Relevant log output

No response

@ederbond ederbond added s/needs-verification Indicates that this issue needs initial verification before further triage will happen t/bug Something isn't working labels May 20, 2022
@HobDev
Copy link

HobDev commented May 20, 2022

Isn't the disposal of the Pages and ViewModels are controlled by DI in Maui ?

@HobDev
Copy link

HobDev commented May 20, 2022

Also this one seems to be related #7329

@PureWeen PureWeen added proposal/open and removed t/bug Something isn't working s/needs-verification Indicates that this issue needs initial verification before further triage will happen labels May 20, 2022
@ederbond
Copy link
Contributor Author

ederbond commented May 20, 2022

@HobDev No, MAUI doesn't care if you implement IDisposable on your page or ViewModel.
It would make our lifes way more ease if MAUI could call the Dispose() method when the page is removed from the navigation stack, on both the Page and the ViewModel if they are implementing IDisposable, similar to what they do when you implement the IQueryAttributable interface on your Page or ViewModel to handle navigation parameters.

@PureWeen
Copy link
Member

PureWeen commented Sep 6, 2022

For now I'd recommend using the Loaded event

@PureWeen PureWeen added this to the Backlog milestone Sep 6, 2022
@ghost
Copy link

ghost commented Sep 6, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@Sebosek
Copy link

Sebosek commented Nov 7, 2022

For now I'd recommend using the Loaded event

Could you please elaborate more about Loaded event? I mean, this is about releasing resources once the page is no longer needed (unsubscribe subscriptions), so I'm curious how the loaded event could help.

@RobTF
Copy link

RobTF commented Feb 1, 2023

I used the Unloaded event on ContentPage which works a treat on Android, but on iOS it is too trigger happy.

Android appears to create an instance of ContentPage, trigger Loaded until the page is popped then trigger Unloaded. If the page participates in tabs, it is not unloaded if the user switches tabs as it is at the front of its logical stack of pages within the tab.

iOS is different, it is similar to the above except if I switch tabs, the page in the tab I am switching away from triggers Unload. When I switch back the same instance of ContentPage is used but a Loaded is triggered instead.

This can make it difficult to reason about the logical lifecycle of a page/control from a memory management point of view. If I have a viewmodel in the BindingContext of a page, I want it to hang around until the page is completely thrown away. I don't know of an event or other mechanism I can rely on to determine this. It does seem to invite memory leaks if one isn't very, very careful.

I'm going to have a play with the navigation side of things to see if there's any mileage there.

@robreno
Copy link

robreno commented Feb 10, 2023

Has any further information been provided per the comment below?

#7329 (comment)

@janseris
Copy link

janseris commented Mar 17, 2023

Came from #7329 (comment)
Registering page (and its ViewModel) as Transient when using Shell still does not create new instances upon visit.

I think it's clear we need to provide some more clarity on how this works exactly and we're working on it.

@jfversluis Is there any "clarification" now after 1 year?

@RobTF
Copy link

RobTF commented Apr 26, 2023

I'm still in a position where I'm struggling to clean up view models as I can't detect when a page is truly finished with. Any guidance would be very welcome!

@albyrock87
Copy link
Contributor

I found some answers in the source code and by doing some testing

  • Singleton lives forever (Application level)
  • Scoped means that the services are tight to the Window which for example on Android means the Activity
    Long story short: on Mobile this is equal to Singleton
  • Transient every time it is requested, creates a new instance: you need to dispose it yourself
    I tested this with Shell and it creates a new instance every time, obviously both Page and ViewModel need to be registered as Transient

Pay attention to this though: in contrast with ASP.NET Core you can request Scoped services from Singleton pages.
The singleton context has its own Scope which is different from the Scoped one.
I highly suggest to not use the Scoped lifetime as it works in unexpected ways (again, it shouldn't be possible to request a Scoped service from Singleton scope).

@ksoftllc
Copy link

@albyrock87 So the MAUI framework never ever disposes any transient items for you? That seems insane. As @ederbond suggested, there should be something in the framework that disposes views when they are no longer in the navigation stack. It will take a lot of extra code and thought to correctly dispose manually all throughout the application.

@RobTF
Copy link

RobTF commented May 11, 2023

I agree, it's seemingly a gaping hole but MAUI isn't the first to do this - WPF and others are the same. You tend to find it in UI frameworks and you end up with stuff like multiple implementations of "weak event handlers" etc.

We run into the event problem where a UI widget needs to listen to something but ended up getting around most of the time using the IMessenger stuff here.

However this does not help when UI components do stuff like create timers or other objects which need cleaning up. In this case you're basically SOL unless you use app-specific trickery to implement cleanup/disposal logic yourself. This is very, very error prone and even taking care throughout our app development, I'm fairly certain there'll be things we've missed.

This was obviously identified as an issue back with Xamarin, due to things such as the LifecycleEffect (see here) being created. As MAUI has rejected effects in favour of handler-based solutions this never came across, but amazingly no alternative was provided.

When I spotted the documentation on handlers I thought I'd found a solution as I assumed that you could correlate the removal of the handler to the "disposal" of the C#/MAUI backing object. However, as per my comment above this is not quite the case as on some platforms the handler can be detached but the C#/MAUI object re-used later, having another handler attached to the same instance. In addition I have seen handlers not be removed/set to null despite the object clearly not being used any longer. This behaviour is not specifically documented so I don't know if it is a bug with the framework or simply a documentation oversight.

It's all a bit confusing!! I think we just need guidance on how to tell when individual Views/Pages can be classed as "done with" so we can tear them down properly. To be honest I get the feeling this is an area the MAUI team have had trouble with themselves due to platform differences, and even they probably don't know all the nuances of it themselves.

@albyrock87
Copy link
Contributor

@ksoftllc I will probably publish a tiny library to handle all of this in the proper way in a couple of weeks. I'm building something that sits on top of Microsoft implementation, without creating a whole new framework/pattern.
As soon as it's ready I'll post here.

@RobTF
Copy link

RobTF commented May 11, 2023

@albyrock87 That sounds really interesting - I'd like to see how you've overcome the issues we've been discussing. If you need testers let me know.

@Quaybe
Copy link

Quaybe commented Aug 4, 2023

Another one for the seemingly endless backlog. They haven't released many MAUI updates the last few months, safe to say I'm a touch nervous.

@ederbond
Copy link
Contributor Author

ederbond commented Aug 4, 2023

Just pinged the team and I hope the come here and tell us a workaround or something... It's almost impossible to maintain the app state healthy if we can't dispose stuffs properly on VM. Specially when you're working with https://okazuki.jp/ReactiveProperty/features/ReactiveProperty.html which I love and I've been using since 2018 on all of my apps along with prism library which allowed me to correctly dispose my VM's. But now since prism is currently not working on MAUI with Shell I have no alternative.

@hartez
Copy link
Contributor

hartez commented Aug 7, 2023

Can you explain more about your scenario? What do you have on a viewmodel which requires explicit Disposal? Are you experiencing memory issues because of long-lived objects? Are you experiencing them with the latest .NET preview versions?

@ederbond
Copy link
Contributor Author

ederbond commented Aug 8, 2023

@hartez
I do make a heavy use of reactive programing using these 2 libraries:
https://github.com/dotnet/reactive
https://okazuki.jp/ReactiveProperty/

So on my ViewModels, I have a lot of IObservable and then inside of my VM's I do subscribe to these observables to react on changes between my VM and some business services that I have. The problem becomes when I have a Service that was registered as Singleton.

Follows a sample that reproduces the issue:

https://github.com/ederbond/PleaseDisposeMe/blob/master/README.md

Steps to reproduce the issue:

  1. Set a breakpoint on Page2ViewModel line 27
  2. Run the app on debug mode
  3. Click on the button called "Notify Now"
  4. You'll see that the label with the current time is updated on the screen. (cool)
  5. Click "Go To Page 2"
  6. You'll see the app will hit your break point a single time (cool)
  7. Go back to page 1
  8. Click on the button called "Notify Now"
  9. You'll see that your break point on Page2V is still getting called wich is bad cause that page was registered as Transient on MauiProgram
  10. Then Navigate again to Page 2
  11. You'll see that your break point will be hit one time again (cool)
  12. Go back to Page 1 and click on the button called "Notify Now"

You'll see that your breakpoint will stop 2x. And if you go back and forth you'll see that your break point will be hitted several times. That's because your Transient ViewModel is still live and listening for data coming from that observable. I my guess is that tha VM will never ever be disposed because it holds subscription from my SingletonService. In the past time of Xamarin.Forms it wasn't an issue for me cause I wasn't using Shell and I was using Prism Library which offers me an interface called IDestructible that when implemented on my VM was always calling a void Destroy() method for me. But since prism is not supporting Maui nor Shell (Dan Seigel already told me that they doesn't even have a plan to support shell in the future). So if someone decides to make Reactive Programing on Maui with Shell you're on a big trouble.
This examples clearly shows that without a proper easy and reliable extension point from the MAUI framework to dispose objects inside a VM your app will have serious memory leaks.

Note that this problem has nothing to do specifically with these 3rd party libraries that I'm using. The same will happen whenever you try to implement the Observable design pattern on your own. Cause your Observable object (Usually a Singleton service) will hold references to it's observers (which are hold by VM).

Having walking dead View Models listening to events from long lived observers not just cause memory leak, but also causes serious business logic issues and random crashed depending of the state it will eventually takes.

@RobTF
Copy link

RobTF commented Aug 8, 2023

Can you explain more about your scenario? What do you have on a viewmodel which requires explicit Disposal? Are you experiencing memory issues because of long-lived objects? Are you experiencing them with the latest .NET preview versions?

Some of our views might create graphics objects for effects, animations etc. which allocate memory and require clean up when they're done with.

In addition, view models might bind up to singleton service objects, bind to native handlers/create native objects which require clean-up, create timers, background threads or other unmanaged support components.

Not stuff which affects "Hello World" level apps, but as soon as your app goes beyond the basics, these things crop up pretty quickly.

@odahan
Copy link

odahan commented Apr 8, 2024 via email

@janseris
Copy link

janseris commented Apr 8, 2024

Wait so how is this done in other XAML UI frameworks with DI? Is it a general issue of the DI container?

@odahan
Copy link

odahan commented Apr 8, 2024 via email

@chrisg32
Copy link

chrisg32 commented Apr 8, 2024

+1 for an interface. This is the lowest hanging fruit: A reliable way knowing when a view is removed and then we can handle the rest e.g. IViewLifecycleAware. If we could get an this interface, I think that would solve all our problems.

One step further with be a way to decorate the shell with a service that manages the lifecycle of the dependency objects e.g. IViewLifecycleManager. The default lifecycle manager could be the current behavior so as not to introduce breaking changes and then provide a second lifecycle manager that calls Dispose on the view model when the pages are removed.

This week I am starting the port of our application to Maui. In our application, memory management is critical. We have been bitten by third party navigation stacks in the past and for this iteration we were hoping to keep navigation and DI as simply as possible. That is looking less and less like a possibility.

@ederbond
Copy link
Contributor Author

ederbond commented Apr 8, 2024

This is currently the 2nd most commented issue here on their GitHub, which has been opened like 2 years ago, and MSFT does nothing about it... It's unbelievable! This is what makes us developers to loose faith in MAUI. How can they ignore their developer community for so long?
Last year they marketed Shell as the new way to go by setting it as default on the new project template of VS even knowing it suffers from this bad design architecture that doesn't allow us to easily create a memory/reactive efficient app, and then keep ignoring us like if it wasn`t a big deal. This is what happens when you just create "Hello World" apps to showcase on conference shows instead of dogfooding your own stuff.

@taublast
Copy link
Contributor

taublast commented Apr 9, 2024

A very interesting material to consider when thinking about a core-level solution: #18366 (comment)

@dzebas
Copy link

dzebas commented Apr 11, 2024

Any chance this is going to be ever looked at? It's such a fundamental issue, any serious app can't be made without it. We started moving our huge enterprise app from Xamarin.Forms to Maui just because Microsoft is forcing everyone, but issues like this makes us very nervous.

@ederbond
Copy link
Contributor Author

Ask @davidortinau, he was supposed to know it.

@janseris
Copy link

janseris commented Apr 11, 2024

Looking at what @davidortinau is doing these days, I found his project where there is an interesting call:

https://github.com/davidortinau/SentenceStudio/blob/main/src/SentenceStudio/MauiProgram.cs

builder.Services.AddTransientWithShellRoute<TPage, TViewModel>()
Maybe that could be of your interest. Don't have time to test it.

Another thing that you can notice is that the project uses Service Locator antipattern. I wonder why.
Example (https://github.com/davidortinau/SentenceStudio/blob/main/src/SentenceStudio/Pages/Vocabulary/AddVocabularyPageModel.cs):

public AddVocabularyPageModel(IFilePickerService picker, IServiceProvider service)
{
    this.picker = picker;
    _vocabService = service.GetRequiredService<VocabularyService>();
}

@ederbond
Copy link
Contributor Author

builder.Services.AddTransientWithShellRoute<TPage, TViewModel>()

Doesn't help at all.

@pme442
Copy link

pme442 commented Apr 11, 2024

I keep seeing big issues like this getting moved to the Backlog and it makes me nervous, too. I'm 3 months into converting a rather complex app from Xamarin Forms to MAUI and I am not even close to having it production-ready. I've lost a lot of features and functionality that I had in the Xamarin Forms app, and I am spending way too much time figuring out and implementing workarounds. Now I'm trying to get a handle on this memory management problem because it is definitely going to be a show stopper.

@dzebas
Copy link

dzebas commented Apr 12, 2024

Tried suggestion by @dansiegel just out of interest, but OnDetachingFrom never gets triggered?

I also tried using Unloaded event, but on iOS it calls it even when you push another page top (actually calls event twice!) and also when navigating back, so no idea when going forward or backwards.

I also thought I could use OnNavigateFrom event, but for some dumb reason NavigatedFromEventArgs does not dispose "DestinationPage" property, it's marked as Internal :(

@memis1970
Copy link

@eder-em I'd love to know if you've managed to get it working using Flyout, DI and MVVM (Community Toolkit) without Shell. After many frustrating hours trying, I still haven't managed it.

@albyrock87
Copy link
Contributor

albyrock87 commented Apr 12, 2024

I see you're all asking to use Shell with IDisposable and I fully understand your frustration/concerns, so I wonder if you've read this message by PureWeen (Microsoft) where the situation is well explained.

For now - as suggested - if you don't want to spend countless hours (trust me) to cook a solution yourself, you can try the library I've built to solve this situation and many other things: it also has an embedded leak detector while debugger is attached.
It is still based on Shell but it provides Scoped context on every page so that you can dispose what you have to.

If you don't like that for some reasons, I'd like to hear your opinion by opening an issue there.

And obviously you can always try out many of the other fantastic libraries out there which have their own navigation handling strategy (not based on Shell).

@bkaankose
Copy link
Contributor

We're primarily not focusing on this issue because we're heavily focused on quality and Xamarin migration. Nothing changed here between Xamarin and MAUI, so it's just not a priority right now. That being said, I think there's some easy net9 wins here.

@PureWeen

If the team priority is not shifted to critical issues/needs like this case sooner or later there is no point of team to work on Xamarin migration. You can just partner with Google or Facebook to make a new partially working tool to migrate apps from Xamarin to Flutter or React Native. No joke...

What is the point of migrating to MAUI when you can't even guarantee one of the most critical things like page lifecycle? People want literally simple things that work reliable in supported platforms. I don't know what your metrics tell you (also don't care because they are mostly misleading the management) but developers care only about 2 platforms: iOS and Android.

  • Page is created. (ctor)
  • Page is navigated to. (I was somewhere else, now this is the page I see)
  • Page is navigated away. (I was here, now I'm going somewhere else. I may come back to it or not)
  • Page is destroyed. (I'm done with this)

Finito. Prism does this well. I wonder why this new shiny framework cannot manage something that 10+ years library is handling so well. Treat each NavigationPage as Frame and handle the stack gracefully.

I know you guys working hard to on the issues, lack of resources and bad management/architectural decisions already. For once, please stop listening people who has no idea about what they do in Xamarin/MAUI and give an ear to those who are trying to do things systematically in 10 levels deep.

@brianlagunas
Copy link

Thanks @eder-em for the recommendation and confidence. @davidortinau you can send the job offer letter to @dansiegel and myself to support@prismlibrary.com. We'll be happy to help improve things 😄

I would like to point out a few clarifications about the statements made about Prism. First, the Patterns and Practices team, which was made up of MS employees, contractors, and community members like myself, were responsible for the initial creation of Prism for WPF. Prism later had a brief attempt at a Windows Store (UWP) version which failed spectacularly along with the UWP framework itself. I then took complete ownership of the project and personally created the XF version from scratch. This is the time Dan came onboard and he is responsible for the MAUI version we have today using the XF version as inspiration. It's been a fun and interesting journey.

It is true that Prism does now have a paid commercial license. However, it is still free for any company/person that meets the community license requirements. We understand this doesn't help everyone, but Prism does solve a lot of the problems that are being mentioned in this thread, which is a strong motivation for companies purchasing a commercial license.

As to this issue specifically, Prism was not able to use the existing IDisposible interface due to the complexities around the intricacies of .NET and garbage collection. Which is why we had to introduce a new interface called IDestructible. Maybe the MAUI team can take a similar approach.

Better yet, Dan and I can sell Prism to Microsoft and then just make it the default behavior. What do you say MS? Ready for another acquisition? LOL

@dzebas
Copy link

dzebas commented Apr 13, 2024

I came up with this solution/hack for now until Maui team can implement something better internally. Seems to be working fine for our project.

Not using IDispose, but created our own IDestructible interface. Basically in our base page when OnNavigatedFrom is called I check if page is still present in either navigation or modal stacks, if yes do nothing as it means moving forward, otherwise call Destroy if page/viewmodel implements interface.

protected override void OnNavigatedFrom( NavigatedFromEventArgs args )
{
    base.OnNavigatedFrom( args );
    
    // is page still present
    if (Shell.Current.Navigation.NavigationStack.Any( p => p == this ) || 
        Shell.Current.Navigation.ModalStack.Any( p => p == this )) 
        return;

    var page = this as IDestructible;
    var vm = _viewModel as IDestructible;
    
    // nothing to destroy
    if (page is null && vm is null)
        return;

    vm?.Destroy();
    BindingContext = null;
    page?.Destroy();
}

@ederbond
Copy link
Contributor Author

Hi @memis1970 I'm @eder-em (I have 2 accounts here).
Here is the framework that I have created with my friend Leo to workaround the issue.

https://github.com/naveasy/Naveasy

@pictos
Copy link
Contributor

pictos commented Apr 14, 2024

Tried suggestion by @dansiegel just out of interest, but OnDetachingFrom never gets triggered?

I also tried using Unloaded event, but on iOS it calls it even when you push another page top (actually calls event twice!) and also when navigating back, so no idea when going forward or backwards.

I also thought I could use OnNavigateFrom event, but for some dumb reason NavigatedFromEventArgs does not dispose "DestinationPage" property, it's marked as Internal :(

Try with the PlatformBevahior, by default Behaviors doesn't call OnDetaching at framework level (since XF). The new PlatformBehavior does, so it will best to use it instead. The API for xaml will not change.

@dzebas
Copy link

dzebas commented Apr 15, 2024

Hi @pictos, thank you, PlatformBevahior works, but I found an issue (same in my code above) - OnDetachedFrom is called on current page when you push another page on top and also when changing TabBar tabs. No idea what it is detaching from if it's still in navigation stack.

Looks like you can't win with Maui.

@memis1970
Copy link

An acquisition of PRISM including the current maintainers of PRISM to help lead the project instead of @davidortinau would be my dream 😂.

Seconded! :-)

@ederbond Thanks for pointing out your library! I'll take a look. Appreciate it.

@brianlagunas
Copy link

Hi @memis1970 I'm @eder-em (I have 2 accounts here). Here is the framework that I have created with my friend Leo to workaround the issue.

https://github.com/naveasy/Naveasy

If you're going to copy Prism code, you can at least give Prism the credit and not to try to pass it off as your own work. I understand that the official Prism project is no longer MIT, and businesses that make more than $1M a year want an alternative free version because they don't want to support the official framework. However, assuming you at least used the v8 MIT version of the code and didn't purposefully break the new Prism license, it would be great if you could at least acknowledge the code you are using from Prism. Thank you.

@davidortinau
Copy link
Contributor

Thanks all for the good discussion. As written, this proposal is not aligned with where we are currently investing.

To narrow the conversation, @PureWeen has drafted 2 specs to consider for .NET 9.

#21814
#21816

@davidortinau davidortinau added t/enhancement ☀️ New feature or request proposal/not-planned and removed proposal/open s/verified Verified / Reproducible Issue ready for Engineering Triage labels Apr 15, 2024
@ederbond
Copy link
Contributor Author

Hi @memis1970 I'm @eder-em (I have 2 accounts here). Here is the framework that I have created with my friend Leo to workaround the issue.

https://github.com/naveasy/Naveasy

If you're going to copy Prism code, you can at least give Prism the credit and not to try to pass it off as your own work. I understand that the official Prism project is no longer MIT, and businesses that make more than $1M a year want an alternative free version because they don't want to support the official framework. However, assuming you at least used the v8 MIT version of the code and didn't purposefully break the new Prism license, it would be great if you could at least acknowledge the code you are using from Prism. Thank you.

Hi @brianlagunas I have updated the readme.md of Naveasy to give the proper credits to the PRISM team.

@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests