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

RefreshView can have a memory leak with a custom ICommand #16124

Open
jonathanpeppers opened this issue Jul 12, 2023 · 3 comments · May be fixed by #26044
Open

RefreshView can have a memory leak with a custom ICommand #16124

jonathanpeppers opened this issue Jul 12, 2023 · 3 comments · May be fixed by #26044
Labels
area-controls-refreshview RefreshView delighter delighter-sc memory-leak 💦 Memory usage grows / objects live forever (sub: perf) platform/android 🤖 platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst platform/windows 🪟 s/triaged Issue has been reviewed t/bug Something isn't working
Milestone

Comments

@jonathanpeppers
Copy link
Member

Description

This problem can be illustrated with a test such as:

main...jonathanpeppers:RefreshViewLeak

If you create a custom ICommand on something like a singleton ViewModel, and use it on a RefreshView, the RefreshView will live forever.

I think this is lower priority, as the customer samples I've seen use Command or Command<T>:

public event EventHandler CanExecuteChanged
{
add { _weakEventManager.AddEventHandler(value); }
remove { _weakEventManager.RemoveEventHandler(value); }
}

And WeakEventManager sidesteps this problem.

If we fix this, it might be worth auditing all properties of type ICommand.

Steps to Reproduce

  1. Create a custom ICommand
  2. Use it on RefreshView
  3. RefreshView lives forever

Link to public reproduction project repository

main...jonathanpeppers:RefreshViewLeak

Version with bug

8.0.0-preview.5.8529

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

All

Did you find any workaround?

Use Command.

Relevant log output

No response

@jonathanpeppers jonathanpeppers added t/bug Something isn't working memory-leak 💦 Memory usage grows / objects live forever (sub: perf) labels Jul 12, 2023
@jonathanpeppers jonathanpeppers added this to the Backlog milestone Jul 12, 2023
@ghost
Copy link

ghost commented Jul 12, 2023

We've added this issue to our backlog, and we will work to address it as time and resources allow. If you have any additional information or questions about this issue, please leave a comment. For additional info about issue management, please read our Triage Process.

@ghost ghost added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Jul 13, 2023
@samhouts samhouts modified the milestones: Backlog, .NET 8 GA Aug 16, 2023
@jaosnz-rep jaosnz-rep added the s/triaged Issue has been reviewed label Jan 31, 2024
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
@dk-mushiyoke
Copy link

dk-mushiyoke commented Jun 13, 2024

Another workaround that I have found, in xaml give refresh view an x:Name and in code behind subscribe to the page Unloaded event and set RefreshView.Command to null

public MainPage()
{
    InitializeComponent();

    Unloaded += MainPage_Unloaded;
}

private static void MainPage_Unloaded(object? sender, EventArgs e)
{
    // Clear refresh view command to avoid memory leaks
    if (sender is MainPage mainPage && mainPage.MyRefreshView?.Command != null)
    {
        mainPage.MyRefreshView.Command = null;
    }
}

@pictos
Copy link
Contributor

pictos commented Nov 21, 2024

From my investigation, this will occur to any control that uses the CommandElement methods. The reason is because when the control is not used the anymore the OnCommandChanging method isn't called, neither the OnCommandChanged method, so it doesn't unsubscribre the CanExecuteChanged event, causing the control to leak.

I could find a solution for that and it's on referenced PR for this issue. But maybe the team wants to revisit how this machinery works and have some cleanup point for this kind of situation, something like DisconnectHandler.

@pictos pictos linked a pull request Nov 21, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-refreshview RefreshView delighter delighter-sc memory-leak 💦 Memory usage grows / objects live forever (sub: perf) platform/android 🤖 platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst platform/windows 🪟 s/triaged Issue has been reviewed t/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants