maui icon indicating copy to clipboard operation
maui copied to clipboard

.Net 8/ 9 Memory leaks

Open Zack-G-I-T opened this issue 1 year ago • 7 comments

Description

We are having some memory issues in our app where the app gets slower after some time and noticed in a gcdump file that all our pages have multiple counts.

We created a dummy project with a second transient page, and added a buton with a method to go back, and call dispose method as well. However in the gcdump there are still multiple counts of this page.

Our question is how are transient pages managed in .NET MAUI? We tested in .net 8 and 9 Transient View/ViewModels are not cleaned up.

How do static variables affect memory management in .NET MAUI? If I am injecting a Singleton Service into a ViewModel will this be cleaned up or also the service has to be transient?

This is a basic default project with no viewmodels and minimal dependency injection, so what is causing the page to be kept in memory?

Image

Steps to Reproduce

  1. Open referenced repo
  2. Create a gcdump file after opening and closing the second page
  3. See multiple counts of Second Page

Link to public reproduction project repository

https://github.com/Zack-G-I-T/MauiMemoryIssue

Version with bug

9.0.0-rc.2.24503.2

Is this a regression from previous behavior?

No, this is something new

Last version that worked well

Unknown/Other

Affected platforms

Android

Affected platform versions

No response

Did you find any workaround?

No response

Relevant log output

No response

Zack-G-I-T avatar Oct 21 '24 16:10 Zack-G-I-T

We've found some similar issues:

  • #21403 , similarity score: 81%
  • #23537 , similarity score: 81%

If any of the above are duplicates, please consider closing this issue out and adding additional context in the original issue.

Note: You can give me feedback by 👍 or 👎 this comment.

similar-issues-ai[bot] avatar Oct 21 '24 16:10 similar-issues-ai[bot]

Hi we have the same issues and despite what we do we have memory leaks - I would expect my pages marked as transient to be cleaned up. Any advice? @jonathanpeppers , is this because of https://github.com/dotnet/maui/issues/8538.

gabsamples6 avatar Oct 22 '24 09:10 gabsamples6

Are you also using IDisposable on those transient pages?

  • https://github.com/dotnet/maui/issues/21816

I am not a fan of DI, in general, so I might not be best to comment about its usage. 😄

jonathanpeppers avatar Oct 22 '24 13:10 jonathanpeppers

@jonathanpeppers Yes I am using DI and I am using Idisposable on those transient pages but they get not cleaned up either.

gabsamples6 avatar Oct 22 '24 17:10 gabsamples6

I think the current behavior is that transient IDisposable won't go away, but @PureWeen might know more details.

jonathanpeppers avatar Oct 22 '24 19:10 jonathanpeppers

Yea, that's how IDisposable works by default with MSFT dependency Injection.

https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection-guidelines#disposal-of-services

PureWeen avatar Oct 23 '24 20:10 PureWeen

Is this a nice simple workaround or a terrible idea?


    public App(IServiceProvider serviceProvider)
	{
		InitializeComponent();

		var loginPage = serviceProvider.GetRequiredService<LoginPage>();
		MainPage = new NavigationPage(loginPage);
		((NavigationPage)MainPage).Popped += PagePopped;
    }

    private void PagePopped(object? sender, NavigationEventArgs e)
    {
        (e.Page as IDisposable)?.Dispose();
    }

tele-bird avatar Oct 29 '24 20:10 tele-bird

Is this a nice simple workaround or a terrible idea?

`

public App(IServiceProvider serviceProvider)
{
	InitializeComponent();

	var loginPage = serviceProvider.GetRequiredService<LoginPage>();
	MainPage = new NavigationPage(loginPage);
	((NavigationPage)MainPage).Popped += PagePopped;
}

private void PagePopped(object? sender, NavigationEventArgs e)
{
    (e.Page as IDisposable)?.Dispose();
}

`

This is something that depends on your scenario... if your pages are transient that should work... Or maybe using the OnNavigatedFrom/To to control those init and clean-up life cycles...

In general your approach looks good, just remember to unsubscribe the event, since you're subscribing it from App class, and it will live forever it can prevent your page to be garbage collected

pictos avatar Oct 29 '24 23:10 pictos

@pictos thanks for the feedback. Yes, all the pages are registered via AddTransient<T>() in the DI container. A good reminder to -= the PagePopped delegate. In our case, that one new NavigationPage(LoginPage) instance lives for the entire duration of the app, so we're good.

tele-bird avatar Oct 30 '24 14:10 tele-bird

The DI guidelines says this about disposal of services:

The container is responsible for cleanup of types it creates, and calls Dispose on IDisposable instances.

Following this guidance, I found that creating a new IServiceScope for resolution of the pushed pages, and then popping and disposing the IServiceScope instance when the page was popped yielded the desired results. Because this approach follows the guidance, this feels legit. So, maybe this isn't a bug at all?

Note here that I'm only managing the scope for the NavigationStack, but I could easily do the same for the ModalStack (for disposal of modal pages).

public class NavigationService : INavigationService
{
    INavigation StackNavigation => Microsoft.Maui.Controls.Application.Current!.MainPage!.Navigation;

    // View model to view lookup - making the assumption that view model to view will always be 1:1
    private readonly Dictionary<Type, Type> _viewModelViewDictionary = new Dictionary<Type, Type>();
    private readonly IServiceProvider serviceProvider;
    private readonly Stack<IServiceScope> serviceScopeStack = new Stack<IServiceScope>();

    public NavigationService(IServiceProvider serviceProvider)
    {
        this.serviceProvider = serviceProvider;
    }

    public void Register<TBaseViewModel, TBaseContentPage>()
        where TBaseViewModel : BaseViewModel
        where TBaseContentPage : BaseContentPage
    {
        if (!_viewModelViewDictionary.ContainsKey(typeof(TBaseViewModel)))
        {
            _viewModelViewDictionary.Add(typeof(TBaseViewModel), typeof(TBaseContentPage));
        }
    }

    public async Task PopAsync(bool animate = false)
    {
        var page = await StackNavigation.PopAsync(animate);
        var scope = serviceScopeStack.Pop();
        scope.Dispose();
    }

    public async Task PopToRootAsync(bool animate = false)
    {
        await StackNavigation.PopToRootAsync(animate);
        var scopeToPopAndDispose = serviceScopeStack.Pop();
        do
        {
            scopeToPopAndDispose.Dispose();
            scopeToPopAndDispose = serviceScopeStack.Pop();
        }
        while(scopeToPopAndDispose != null);
    }

    public Task PushAsync<TViewModel>(bool animate = false, Action<TViewModel>? initAction = null) where TViewModel : BaseViewModel
    {
        var viewType = _viewModelViewDictionary[typeof(TViewModel)];
        var scope = serviceProvider.CreateScope();
        var page = (BaseContentPage<TViewModel>)scope.ServiceProvider.GetRequiredService(viewType);
        serviceScopeStack.Push(scope);
        initAction?.Invoke(page.ViewModel);
        return StackNavigation.PushAsync(page, animate);
    }
}

tele-bird avatar Oct 30 '24 17:10 tele-bird

If it is transient, it should be release and the DI container won't hold it. But, maybe this is not the page but the handler holding the page?

That is a very narrow hot path. Anything else in there that looks like 5 or 10 or something?

mattleibow avatar Nov 18 '24 19:11 mattleibow

Is this a nice simple workaround or a terrible idea?

public App(IServiceProvider serviceProvider)

{ InitializeComponent();

  var loginPage = serviceProvider.GetRequiredService<LoginPage>();
  MainPage = new NavigationPage(loginPage);
  ((NavigationPage)MainPage).Popped += PagePopped;
}

private void PagePopped(object? sender, NavigationEventArgs e)
{
    (e.Page as IDisposable)?.Dispose();
}

This probably won't help. The DI container is still going to retain a reference to Page because your Pages implement IDisposable. Calling Dispose yourself does nothing. It doesn't set any flags or signal to the DI container that it's been called.

Unless there's some specific reason you need to use IDisposable, I would just not implement `IDisposable.

If you need logic to run when a page is popped then I'd just add some method to call "NavigatedAway" or "Popped" and call that, or look at using EventToCommandBehaviors from MCT.

PureWeen avatar Nov 18 '24 19:11 PureWeen

The DI guidelines says this about disposal of services:

The container is responsible for cleanup of types it creates, and calls Dispose on IDisposable instances.

Following this guidance, I found that creating a new IServiceScope for resolution of the pushed pages, and then popping and disposing the IServiceScope instance when the page was popped yielded the desired results. Because this approach follows the guidance, this feels legit. So, maybe this isn't a bug at all?

Note here that I'm only managing the scope for the NavigationStack, but I could easily do the same for the ModalStack (for disposal of modal pages).

public class NavigationService : INavigationService { INavigation StackNavigation => Microsoft.Maui.Controls.Application.Current!.MainPage!.Navigation;

   // View model to view lookup - making the assumption that view model to view will always be 1:1 private readonly Dictionary<Type, Type> _viewModelViewDictionary = new Dictionary<Type, Type>(); private readonly IServiceProvider serviceProvider; private readonly Stack<IServiceScope> serviceScopeStack = new Stack<IServiceScope>();

public NavigationService(IServiceProvider serviceProvider)
{
    this.serviceProvider = serviceProvider;
}

public void Register<TBaseViewModel, TBaseContentPage>()
    where TBaseViewModel : BaseViewModel
    where TBaseContentPage : BaseContentPage
{
    if (!_viewModelViewDictionary.ContainsKey(typeof(TBaseViewModel)))
    {
        _viewModelViewDictionary.Add(typeof(TBaseViewModel), typeof(TBaseContentPage));
    }
}

public async Task PopAsync(bool animate = false)
{
    var page = await StackNavigation.PopAsync(animate);
    var scope = serviceScopeStack.Pop();
    scope.Dispose();
}

public async Task PopToRootAsync(bool animate = false)
{
    await StackNavigation.PopToRootAsync(animate);
    var scopeToPopAndDispose = serviceScopeStack.Pop();
    do
    {
        scopeToPopAndDispose.Dispose();
        scopeToPopAndDispose = serviceScopeStack.Pop();
    }
    while(scopeToPopAndDispose != null);
}

public Task PushAsync<TViewModel>(bool animate = false, Action<TViewModel>? initAction = null) where TViewModel : BaseViewModel
{
    var viewType = _viewModelViewDictionary[typeof(TViewModel)];
    var scope = serviceProvider.CreateScope();
    var page = (BaseContentPage<TViewModel>)scope.ServiceProvider.GetRequiredService(viewType);
    serviceScopeStack.Push(scope);
    initAction?.Invoke(page.ViewModel);
    return StackNavigation.PushAsync(page, animate);
}

}

This is the more correct approach. The one caveat here is that if you start trying to resolve MAUI types you might run into issues. For example, we register the IDispatcher as a scoped service so your container will create a new instance of IDispatcher.

We'd like to implement what you have here as a feature of MAUI at some point but just haven't quite had time to unfortunately.

@albyrock87 has a helpful navigation library here that probably does a lot of what you're looking for.

PureWeen avatar Nov 18 '24 19:11 PureWeen