dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.05k stars 1.73k forks source link

[Maui Embedded] DisconnectHandler not called when view is closed/disposed #18366

Open ultimategrandson opened 11 months ago

ultimategrandson commented 11 months ago

Description

I am assuming this is because of my application utilizing Maui Embedded. I am creating a view as per the documentation. However, when I close the ViewController, it does not call DisconnectHandler.

I noticed this when I was implementing a Barcode Reader (using the ZXing MAUI library), the cleanup code that stops the camera from running is in the DisconnectHandler method. When the view is closed, it does not call this and the camera remains running.

Is there a way to cleanup these handlers?

Steps to Reproduce

No response

Link to public reproduction project repository

No response

Version with bug

7.0.96

Is this a regression from previous behavior?

Yes, this used to work in Xamarin.Forms

Last version that worked well

Unknown/Other

Affected platforms

iOS, 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

ultimategrandson commented 11 months ago

I have found a potential workaround, if I put this code in my ViewController, it will disconnect the handlers when I don't need them anymore:

foreach (var element in MauiPage.GetVisualTreeDescendants().OfType<IElement>()) element.Handler.DisconnectHandler();

Is this the recommended way of handling this?

nll commented 11 months ago

From this warning on the documentation it seems that is the expected behavior:

The DisconnectHandler method is intentionally not invoked by .NET MAUI. Instead, you must invoke it yourself from a suitable location in your app's lifecycle. For more information, see Native view cleanup.

(I was expecting it to be called as well, I only discover this recently)

from https://learn.microsoft.com/en-us/dotnet/maui/user-interface/handlers/create#create-the-platform-controls

ultimategrandson commented 11 months ago

I did not know this. Do any of the standard maui controls need disconnecting?

It would be nice if the handlers could indicate that they require cleanup, and then on the parent have a DisconnectAllHandlers that cleans up what needs to be cleaned up.

PureWeen commented 11 months ago

The problem is that we can't know when you want to disconnect the handler.

For example, if you're moving a view from one layout to another layout you might not want it to disconnect.

The guidance here is to use loaded/unloaded or the platform events related to being part of the visual tree to achieve what you want.

You know more about your life cycle than we do :-)

ghost commented 11 months ago

Hi @ultimategrandson. We have added the "s/needs-info" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

ghost commented 11 months ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. If it is closed, feel free to comment when you are able to provide the additional information and we will re-investigate.

ultimategrandson commented 11 months ago

The problem is that we can't know when you want to disconnect the handler.

For example, if you're moving a view from one layout to another layout you might not want it to disconnect.

The guidance here is to use loaded/unloaded or the platform events related to being part of the visual tree to achieve what you want.

You know more about your life cycle than we do :-)

I accept that we should do the cleanup, its just knowing what actually needs to be cleaned up.

Also, handlers don't implement IDisposable which I would normally intuit to meaning I need to manually clean up the resources of this myself.

VNGames commented 10 months ago

@PureWeen Please answer the question about whether MAUI controls are cleaned automatically and calling DisconnectHandler We have to cleanup our own controls but do we also have to clean up MAUI controls as well?

For example. Entry has some cleanup code. Who is gonna call DisconnectHandler for it? We are told by documentation to clean our controls but nothing about MAUI out of the box controls. Does MAUI clean them up somewhere? https://github.com/dotnet/maui/blob/7acd5c2990aa0241bf896c743d81d7f716b05868/src/Core/src/Handlers/Entry/EntryHandler.Android.cs#L52

cosminstirbu commented 9 months ago

@PureWeen Please answer the question about whether MAUI controls are cleaned automatically and calling DisconnectHandler We have to cleanup our own controls but do we also have to clean up MAUI controls as well?

For example. Entry has some cleanup code. Who is gonna call DisconnectHandler for it? We are told by documentation to clean our controls but nothing about MAUI out of the box controls. Does MAUI clean them up somewhere?

https://github.com/dotnet/maui/blob/7acd5c2990aa0241bf896c743d81d7f716b05868/src/Core/src/Handlers/Entry/EntryHandler.Android.cs#L52

I have the same question.

I did a bit of testing using a custom handler that inherits from ButtonHandler and I did notice that DisconnectHandler is never called. Are we expected to manually call DisconnectHandler (which by the way is a protected method, not a public one) for all existing MAUI controls?

haavamoa commented 9 months ago

Is this for real? I just noticed that our DisconnectHandler was not called, which means we have a lot of memory leakage in our application right now. We are creating a library where we have a lot of event subscriptions going on, where we was 100% sure DisconnectHandler would run when the page was unloaded.

If this is intentional, then this is completely different from the Xamarin way of disposing views, and right now I am not sure how we can create library UI components without having to add a lot of dispose code for each application we create. Please do something about this, add some very clever and easy way for us to dispose our views when the parent page is disposed...

AdamEssenmacher commented 8 months ago

Edit: For anyone coming across this later, this below line of reasoning led me to create: https://github.com/AdamEssenmacher/MemoryToolkit.Maui to automate view cleanup


The problem is that we can't know when you want to disconnect the handler.

For example, if you're moving a view from one layout to another layout you might not want it to disconnect.

The guidance here is to use loaded/unloaded or the platform events related to being part of the visual tree to achieve what you want.

You know more about your life cycle than we do :-)

@PureWeen, I'm finding it hard to accept this line of reasoning.

It's true that we cannot be absolutely certain about a given MAUI developer's intent regarding their view lifecycles.

However, we can be reasonably certain about a given MAUI developer's intent regarding their view lifecycles 99% of the time... We want them cleaned up when they're removed from the ~visual tree~ (edit: navigation stack)--and we don't want to have to think about it!

Put another way, developers should be able to 'opt out' of handler disconnection on unload if needed. Requiring them to 'opt in' is backwards.


@haavamoa, I'm not sure how 'clever' this is yet, as I'm just starting to explore the approach. I've written a couple of attached properties that you and others might find useful.

The first looks like this in use:

<ContentPage ...
             local:HandlerDisconnectBehavior.Cascade="True">

Setting this attached property hooks into the VisualElement's Unload event. When triggered, it walks the visual tree calling DisconnectHandler() on all of its children.

To account for cases where we might want to selectively 'opt out' of this automatic disconnection behavior, a second attached property is provided:

<SomeMovableView ...
             local:HandleDisconnectBehavior.Suppress="True">

If HandleDisconnectBehavior.Cascade encounters an element with this attached property, it will skip it and its children.


public static class HandlerDisconnectBehavior
{
    public static readonly BindableProperty CascadeProperty =
        BindableProperty.CreateAttached("Cascade", typeof(bool), typeof(HandlerDisconnectBehavior), false,
            propertyChanged: CascadeChanged);

    public static readonly BindableProperty SuppressProperty =
        BindableProperty.CreateAttached("Suppress", typeof(bool), typeof(HandlerDisconnectBehavior), false);

    public static bool GetCascade(BindableObject view)
    {
        return (bool)view.GetValue(CascadeProperty);
    }

    public static void SetCascade(BindableObject view, bool value)
    {
        view.SetValue(CascadeProperty, value);
    }

    public static bool GetSuppress(BindableObject view)
    {
        return (bool)view.GetValue(SuppressProperty);
    }

    public static void SetSuppress(BindableObject view, bool value)
    {
        view.SetValue(SuppressProperty, value);
    }

    private static void CascadeChanged(BindableObject view, object oldValue, object newValue)
    {
        if (view is not VisualElement visualElement)
            throw new InvalidOperationException(
                "HandlerDisconnectBehavior.Cascade can only be attached to a VisualElement");

        var attachBehavior = (bool)newValue;
        if (attachBehavior)
            visualElement.Unloaded += OnVisualElementUnloaded;
        else
            visualElement.Unloaded -= OnVisualElementUnloaded;
    }

    private static void OnVisualElementUnloaded(object? sender, EventArgs e)
    {
        if (sender is not VisualElement senderElement)
            return;

        if (GetSuppress(senderElement))
            return;

        var visualTreeElement = (IVisualTreeElement)senderElement;

        Disconnect(visualTreeElement);

        return;

        void Disconnect(IVisualTreeElement element)
        {
            var visualElement = (VisualElement)element;

            if (GetSuppress(visualElement))
                return;

            foreach (IVisualTreeElement childElement in element.GetVisualChildren())
                Disconnect(childElement);

            visualElement.Handler?.DisconnectHandler();
        }
    }
}
haavamoa commented 8 months ago

Nice @AdamEssenmacher , I also found out that UnLoaded can work, and created a small guidance that we will use for our developers in my company. Problem is that that ,as you see from my writings, the invocations are different on Android and iOS, especially problematic on iOS :(

I am referring to this line from my wiki page:

Invoked 2x times when the page was navigated to a new page. (iOS)

This means that you will have to make sure to "reconnect" everything when people navigate back to the page they came from, which is the page that has "disconnected".

AdamEssenmacher commented 8 months ago

I've spent the better part of a week tracking down and fixing memory leaks in both third-party and standard MAUI controls on iOS and Android. Having gone deep down this rabbit hole, I'm starting to make some sense of the situation.

First, let's clarify that the express purpose of calling DisconnectHandler() is not to prevent memory leaks. It is a signal to the handler that the underlying platform view will no longer be needed, allowing the handler to clean up.

This being said, calling DisconnectHandler() does have real-world implications impacting memory leaks. The nature of its impact can depend on the MAUI view/element, its state, and the platform (e.g. Android/iOS).

Before I can explain the different effects DisconnectHandler() can have, it's important to understand the reference relationships between a MAUI page-level view (e.g. ContentPage) and a given native platform view (e.g. AppCompatTextView). Consider the following XAML:

<ContentPage>
   <...>
      <Label Text="..." />
   </...>
</ContentPage>

In MAUI world, the Label view strongly references the ContentPage through the .Parent property chain. Pages also reference their children, so the reference relationship between Page <-> View is circular. This isn't inherently bad. In normal .NET, circular references are handled just fine by the GC. However, in the case that an individual view becomes uncollectible for any reason, then the leak will spread from the view to its parent page, and then to all other views in that page. This is why MAUI apps are leaking so heavily right now. Small leaks that would otherwise have been innocuous over an app's typical lifecycle have an enormously disproportionate effect. This is like discovering a gas leak in an enclosed space by lighting a match.

As an aside, this cascading leak effect can be mitigated by nulling out the Parent property of a leaking view. It won't stop the View itself from leaking, but by breaking the reference to the page you can prevent the leak spreading so massively. This could be a better-than-nothing stopgap for production apps suffering right now.

Next, let's see how an individual View might become leaky by turning to the platform side. Each MAUI view (virtual view) is backed by a Handler, which connects it to the native platform view that is ultimately rendered on the screen. There is a circular reference between the virtual view and the handler. There is usually a circular reference between the platform view and the handler, but I don't think that's strictly necessary.

So our total reference chain looks like this: Page <-> Virtual View <-> Handler <-> Platform View

This extends our cascading leak problem. Meaning, should a leak be introduced by the handler or platform view, the leak will spread through the virtual view to the page.

How DisconnectHandler() comes into play DisconnectHandler() is a virtual method with dozens of implementations across the standard MAUI controls. Then there's the 3rd-party or homebrewed implementations. So there's variation in how it actually behaves. Depending on the platform and implementation it may be that DisconnectHandler has one of the following effects:

muak commented 4 months ago

My workaround is the following:

public class SomeHandler: ViewHandler<SomeView, UIView>
{
    protected override void ConnectHandler(UIView platformView)
    {
        base.ConnectHandler(platformView);

        VirtualView.AddCleanUpEvent();
    }

    protected override void DisconnectHandler(UIView platformView)
    {
        base.DisconnectHandler(platformView);

        platformView.Dispose();
    }
}
public static class HandlerCleanUpHelper
{
    public static void AddCleanUpEvent(this View view)
    {
        if (view is not Element element)
        {
            return;
        }

        Page? parentPage;

        void PageUnloaded(object? sender, EventArgs e)
        {
            view.Handler?.DisconnectHandler();
            if (parentPage is not null)
            {
                parentPage.Unloaded -= PageUnloaded;
                parentPage = null;
            }
        }        

        foreach (var el in element.GetParentsPath())
        {
            if (el is Page page)
            {
                parentPage = page;
                page.Unloaded += PageUnloaded;
            }
        }        
    }

    // From MAUI source
    public static IEnumerable<Element> GetParentsPath(this Element self)
    {
        Element current = self;

        while (!IsApplicationOrNull(current.RealParent))
        {
            current = current.RealParent;
            yield return current;
        }
    }

    public static bool IsApplicationOrNull(object? element) =>
            element == null || element is IApplication;
}
muak commented 4 months ago

Nobody wants to re-use the controls on a page after the page has been destroyed. At the very least, I think it is the framework's responsibility to call DisconnectHandler when a page is destroyed.

For the time being, I will try to deal with this in @AdamEssenmacher workaround, but I hope it will be improved as MAUI.

haavamoa commented 3 months ago

We faced issues with effect and behaviors causing leaks as well. To fix that you will have to do a .Clear() of the effects / behaviors once the page is not used anymore to get the Deattached methods to be invoked.

We are have also noticed that UnLoaded events run when adding a modal page to the stack when a page is open, so be careful to not clean up a view that is a part of a page that pushes a modal page 😱

Our app is a Shell app, and we can use the Navigated Back events from Shell to know when to clean up instead of Unloaded for views.

I still find it quite disturbing that disconnect/unattached/unloaded is not events that MAUI invokes onve a view is not used anymore, and that we as consumers of MAUI has the responsibility of figuring out a merhod to clean up.

Real life scenario: We have a huge application running in production in hospitals, and the memory leak is so bad (especially on Android) now that they have to restart the app in order to treat patients some times. This cause them to leave the phone and the patient to walk to a computer instead..

This is of course (partially) our (the consumers) responsibility, and we have created patterns and pages that is not very performant or cleaned up, but we need to trust the framework to have some kind of event/method we can trust to use for clean up… In fact, our assumptions was initially that disconnect,unloaded and deattach was methods we could trust, without ever testing it until now :/

We will spend most of our time from August to solve this issue now, and hope it will be better for nurses and doctors 👍🏻

bkaankose commented 2 months ago

Please just introduce a breaking change for DisconnectHandler by changing the signature as well and modify all controls to Dispose/Disconnect themselves when the Page goes out of scope by MAUI. It's obvious that industry don't want to / can't handle something supposedly handled by the platform itself. It's not rocket science to detect in navigation stack that whether the page object is needed or not in app lifetime.

Don't let years of hard work to be gone for nothing. It's an obvious architecture decision mistake going back years ago, but it's never too late to fix it.

nll commented 1 month ago

It seems that after the silence from MAUI team we have something on .NET MAUI updates in .NET 9 Preview 7

Summary