Should Pages be Disposable?

ChaseFlorellChaseFlorell CAInsider, University mod
edited August 2014 in Xamarin.Forms

There seems to be a little bit of issue with pages not being released when their popped. I'm wondering if pages should implement IDisposable?

I posted a comment here, and I wonder if it would work with the current implementation of Xamarin.Forms... IE: would Pop actually call Dispose?

This is just a thought....

public class SecondPage : ContentPage
{
    private Image _img;

    public SecondPage()
    {
        _img = new Image {Source = new FileImageSource {File = "largeImage.jpg"}};

        Content = _img;
    }

    protected override void Dispose() // overrides the base Dispose because [ Page : IDisposable ]
    {
        base.Dispose();

        _img.Source = null;
        _img = null;

        GC.Collect();
    }
}

Posts

  • adamkempadamkemp USInsider, Developer Group Leader mod

    I vote for no. IDisposable is intended for cases where you have unmanaged resources. This isn't one of those cases.

    Instead I would use OnAppearing and OnDisappearing for now. We could also maybe use OnBindingContextChanged if there were clear places in which it could be set to null.

    It would be nice if there were a few more lifetime methods, but it's not clear whose responsibility it would be to call them. Even with your suggestion of using Dispose it's not clear who should call that. Not all pages are used with navigation.

  • ChaseFlorellChaseFlorell CAInsider, University mod

    @adamkemp, I understand your points. I think the Dispose method (if implemented) could be called in the Finalizar, however, maybe IDisposable isn't the right solution, but we need some way of cleaning up these memory leaks.

    http://forums.xamarin.com/search?cat=24&adv=1&search=outofmemory
    http://forums.xamarin.com/discussion/comment/73473/
    https://bugzilla.xamarin.com/show_bug.cgi?id=22510
    https://bugzilla.xamarin.com/show_bug.cgi?id=22162
    https://bugzilla.xamarin.com/show_bug.cgi?id=21995
    https://bugzilla.xamarin.com/show_bug.cgi?id=21840
    https://bugzilla.xamarin.com/show_bug.cgi?id=21995
    http://forums.xamarin.com/discussion/23007/how-to-track-down-outofmemory-exception-on-production-device

    I don't know the inner workings of X.Forms, so I'm not sure why these objects are hanging around. I don't even know if manually nulling stuff out in the Finalizer would solve anything.

    ~MyPage()
    {
        _myListView.ItemsSource = null;
        _myListView = null;
    }
    
  • MihaMarkicMihaMarkic SI ✭✭✭✭

    @adamkemp‌ is correct. IDispose is for unmanaged resources only. And using a finalizer is even worse (just adding a perf hit without any benefit). If pages aren't released after PopXXX then there is a problem with Forms somewhere that should be addressed. In general IPageNavigation implementation is not something well thought to start with.

  • MihaMarkicMihaMarkic SI ✭✭✭✭

    Besides I have a neat feeling that these issues could be binding related.

  • adamkempadamkemp USInsider, Developer Group Leader mod

    By the time you've reached the finalizer there's no point in nulling out that reference because your object is guaranteed not to be holding that image in memory anymore (because your object is being collected already). That code accomplishes nothing.

    In my mind it would make sense if the image object held by the non-renderer side of things (I.e., the page and other cross platform view objects) would be very lightweight, maybe just containing a name or path or whatever is necessary to load the image on the renderer side when needed. It shouldn't hold any large resources in memory because you can't need them outside the renderer anyway. I may be wrong, but maybe a solution like that could solve a lot of these issues. Or maybe there's just a memory leak bug in bindings or something as Miha suggested.

  • YakubuYakubu USMember

    When I play media on a webview on a navigation stack and navigate away from the webview page in question it keeps playing indefinitely and in many cases I cannot go back to that page.This is a big issue that should not be ignored

  • adamkempadamkemp USInsider, Developer Group Leader mod

    That's something that the renderer should take care of. On which platform are you seeing that behavior?

  • nbevansnbevans USMember ✭✭✭
    edited May 2015

    @adamkemp. IDisposable is not for unmanaged resources. That is simply a myth that has perpetuated for too many years in .NET land. Hell, there are plenty of examples of .NET BCL types which implement IDisposable and that never ever touch some unmanaged resource or code at all.

    IDisposable is intended for deterministic disposal. It lets you start the process of freeing resources before the garbage collector which is indeterministic.

    The problem is not so much that Page doesn't implement IDisposable, it is that when a page is popped that internally Xamarin Forms seems to still hold references to it. I suspect they are event handlers which aren't weak referenced, perhaps. So even if pages did implement IDisposable by default, it still wouldn't cure the fact that a Page instance can never actually be garbage collected. IDisposable would simply serve as a means for the developer to do some app-specific cleanup.

    I believe that Page should provide a default implementation of IDisposable (virtual method in C#, default member in F#) but that this should only come after XF has solved the internal memory leak.

  • adamkempadamkemp USInsider, Developer Group Leader mod

    Microsoft says otherwise:

    Implement IDisposable only if you are using unmanaged resources directly.

    I'm not sure which APIs you're talking about, but Microsoft definitely made mistakes early on that unfortunately are not practical to fix. That doesn't make it correct to use the interface today for managed resources.

    The problem is that implementing the IDisposable pattern correctly implies that the Dispose(bool) method will always get called eventually, but it may get called in a finalizer thread. The kinds of cleanup you're likely to do in a Page are totally inappropriate for a finalizer. If your resources can't be cleaned up in a finalizer then they shouldn't be cleaned up using IDisposable at all.

  • YakubuYakubu USMember

    And ..Does onDisappearing even work? It didn't work for me.

  • YakubuYakubu USMember

    I am seeing the behavior on Android. Why is there no OnPopAsync event at least. There is no way to even access the navigation back button as a control.I am not impressed at all.

  • adamkempadamkemp USInsider, Developer Group Leader mod

    OnDisappearing should work. In which case are you seeing it not work?

    You can handle the back button being pressed by overriding OnBackButtonPressed. To handle the pop event look at the events on NavigationPage. That's the correct place to handle popping of pages. For modal pages there are equivalent events on the Application class.

  • JeremyHerbisonJeremyHerbison CAMember ✭✭

    IDisposable is not _only _for unmanaged resources. It can be used to clean up expensive managed objects as well. But otherwise I agree; Pages do not need it.

  • adamkempadamkemp USInsider, Developer Group Leader mod

    Of course it can, but Microsoft explicitly says it shouldn't be, and I already explained the problems it causes. Just because you can doesn't mean you should.

  • nbevansnbevans USMember ✭✭✭
    edited May 2015

    Microsoft were the root cause of this myth about IDisposable, which has been perpetuated ever since v1.0 of .NET. Even within Microsoft's own products they use IDisposable as it was always intended i.e. simply for deterministic disposal. Again: it has nothing to do with managed or unmanaged resources. It is purely concerned with deterministic disposal which is an abstract concept really.

    Think about it logically. If it really was for unmanaged resources, don't you think it would be filed under some Marshalling type or namespace of .NET? That's not where it is. First, IDisposable as an interface is held in the root namespace of System. Second, the using keyword in C# and the use keyword in F# (and the using function in F#) are all language constructs that are provided outside of any form of context around the subject of marshalling to unmanaged resources. Third, IDisposable is available in PCL, Silverlight etc, assemblies (which cannot use unmanaged stuff at all) again further squashing the myth that it was ever intended for unmanaged resources only.

    I don't give a toss what MSDN says really. It is merely a reference, not a gospel. MSDN says a lot of things. Just go read some of their hilarious "best practices" and "patterns & practices" articles. If Microsoft, or any big corp for that matter, says something is best practice then in my experience it usually isn't. If we all followed blindly what MSDN said then we'd all be using "Logging Application Blocks" or some rubbish just to achieve basic stuff.

  • nbevansnbevans USMember ✭✭✭

    "IDisposable is not _only _for unmanaged resources. It can be used to clean up expensive managed objects as well. But otherwise I agree; Pages do not need it."

    If unregistering from an event handler, deterministically - as is typically required for event handlers, is considered "expensive" then so be it. I personally consider that as not expensive at all, but still very very mandatory. I'm sure everybody here is aware of the "event handler pub/sub dependency inversion anti-pattern" right? Seems possibly not: http://thatextramile.be/blog/2008/08/why-you-should-always-unscubscribe-event-handlers/

    I don't like attributing any particular use cases to IDisposable. It is an abstract concept purely for anything that falls under the banner of "deterministic disposal". Of which there are almost limitless examples.

  • adamkempadamkemp USInsider, Developer Group Leader mod

    So Microsoft is wrong about how a thing they invented should be used? Really?

    The namespace is totally irrelevant, and it has to be accessible in PCLs because even if you can't directly work with unmanaged resources you may be using an object that does have unmanaged resources. The interface itself is just a contract. I'm not sure why you bring marshaling in, as unmanaged resources don't imply marshaling.

    The using statement also exists specifically to aid in deterministically freeing unmanaged resources. Microsoft doesn't intend for it to be used to free up memory because it very clearly doesn't actually do that. Calling Dispose on something doesn't free a single byte of managed memory, nor does it in any way make the freeing of managed memory deterministic. If that's what you want from it then you fundamentally misunderstand how the GC works.

    Again, Microsoft has made mistakes. Think of ICloneable as an example. You can't use the existentance of counter examples from Micrsosoft to prove intent. They have stated their intent clearly. Your argument is that we should totally ignore that, and that argument is absurd.

    I don't give a toss what MSDN says really.

    Well I don't give a toss what you say either. You have no credibility. None of your arguments come close to convincing me in the least that I should take your opinion over the clearly stated position of the creators of the interface and the language.

  • nbevansnbevans USMember ✭✭✭

    Jesus wept. CS101 on Xamarin Forums. How did this happen!

    http://en.wikipedia.org/wiki/Dispose_pattern
    http://alvinalexander.com/java/jwarehouse/picocontainer-2.0/org/picocontainer/Disposable.java.shtml
    http://docs.oracle.com/javase/7/docs/api/java/lang/AutoCloseable.html

    Ctrl+F on those for "unmanaged".

    Even the example of the MSDN page goes against the rest of the article. Because StreamReader doesn't use unmanaged resources by itself. It is just a decorator on top of another stream. What if that other stream is a MemoryStream for example? Again, if you are using MemoryStream (and not UnmanagedMemoryStream), then that too contains no unmanaged code or resources. And yet, again, even a MemoryStream implements the interface to support deterministic disposal.

    Database connections don't necessarily imply unmanaged resources are being used and yet find me a database client for .NET that doesn't implement the dispose pattern on its connections. The whole idea of having to "second guess" whether some component you are using is using unmanaged resources or not inside of its private implementation that you have no ability to see, is frankly, utterly ridiculous. That's why this whole myth about the dispose pattern is particularly hilarious to me.

    It's just a design pattern dude. To talk of it like Microsoft invented it and therefore has the ultimate say on what it can and mustn't be used for is ridiculous. MSDN is wrong. And you are wrong to support MSDN when the evidence is so clearly counter to what it is saying.

  • nbevansnbevans USMember ✭✭✭

    Just to reinforce the point that you really SHOULD NOT believe everything MSDN says:

    https://msdn.microsoft.com/en-us/library/b1yfkh5e(v=vs.110).aspx

    What do we have here then... another article on MSDN talking about the dispose pattern and this one says:

    "√ CONSIDER implementing the Basic Dispose Pattern on classes that themselves don’t hold unmanaged resources or disposable objects but are likely to have subtypes that do."

    It doesn't shout this anywhere near loudly enough as clearly the message, in 2015, is still not getting through to the .NET community.

    Meanwhile, developers using other tech stacks don't have any "holds ups" about if and when it is appropriate to implement the dispose pattern. They just get on and do it.

  • adamkempadamkemp USInsider, Developer Group Leader mod

    "√ CONSIDER implementing the Basic Dispose Pattern on classes that themselves don’t hold unmanaged resources or disposable objects but are likely to have subtypes that do."

    Let me paraphrase: if your class doesn't itself have unmanaged resources, but a subclass likely will have unmanaged resources, then because you need to deal with possible unmanaged resources you should use IDisposable. That still only applies if there are likely unmanaged resources involved. It does not apply in the case where there are never any unmanaged resources involved.

    That is precisely why StreamReader is IDisposable: because it might have unmanaged resources, and the client has to be able to handle it if it does. That is painfully obvious, and yet you think that's an argument for your case. Astounding.

    Jesus wept. CS101 on Xamarin Forums.

    You have used so many bad arguments that it's amazing to me that you would have the gall to make that asinine comment. I'm done here.

  • ddotmunchddotmunch DEMember ✭✭

    Interesting discussion. Personally I never hesited implementing IDisposable to deterministically dispose managed resources. And when checking Microsoft's (aka "the creator")' documentation it explicitely states:

    √ DO implement the Basic Dispose Pattern on types containing instances of disposable types. See the Basic Dispose Pattern section for details on the basic pattern.
    If a type is responsible for the lifetime of other disposable objects, developers need a way to dispose of them, too. Using the container’s Dispose method is a convenient way to make this possible.

    But hey, it's a pattern, and as long as you use it consisentently in your code it should be fine, right?

  • adamkempadamkemp USInsider, Developer Group Leader mod

    I'll just repeat this:

    The kinds of cleanup you're likely to do in a Page are totally inappropriate for a finalizer. If your resources can't be cleaned up in a finalizer then they shouldn't be cleaned up using IDisposable at all.

  • TorbenKruseTorbenKruse DEMember ✭✭✭

    Here is one thing though: Microsoft uses IDisposable on ASP.Net MVC to just close HTML tags. So don't be too harsh. :smile:

    using(Html.BeginForm()) // opens the <form> tag
    {
        // Inner HTML
    }  // closes the </form> tag
    
  • adamkempadamkemp USInsider, Developer Group Leader mod

    Like I said, Microsoft has made mistakes. Since then they've come up with guidelines so that we don't repeat those mistakes. There are plenty of other stupid things in their code that violate their own guidelines, but that doesn't make it ok to just ignore their advice.

  • JeremyHerbisonJeremyHerbison CAMember ✭✭

    Jeez you're crusty. What exactly is the problem with using IDisposable this way, other than that it violates some Technet article? As in, what real-world problems does it create?

  • ChaseFlorellChaseFlorell CAInsider, University mod

  • adamkempadamkemp USInsider, Developer Group Leader mod

    The problem was right here, repeated several times already:

    The kinds of cleanup you're likely to do in a Page are totally inappropriate for a finalizer. If your resources can't be cleaned up in a finalizer then they shouldn't be cleaned up using IDisposable at all.

    Any object that implements IDisposable should also have a finalizer, and you can't do most page-related cleanup in a finalizer thread. That's why it's a terrible idea. If you can't run your cleanup code in a finalizer then don't use IDisposable for it.

  • JeremyHerbisonJeremyHerbison CAMember ✭✭

    Nowhere does it say you must include a finalizer for all implementations of IDisposable. For example, if you are holding references to an IDisposable object, you should implement IDisposable but don't need a finalizer.

  • BrianRepettiBrianRepetti USUniversity ✭✭✭

    Why dont you guys set a test project and run a memory analyzer against it?

    Anyways, you should set Content = null in OnDisappearing and set Content in OnAppearing

  • ChaseFlorellChaseFlorell CAInsider, University mod

    When I originally asked this, there was some memory leaks that occurred after a page was popped (IE: after OnDisappearing). I asked because I wanted to be able to ensure everything was cleaned up. This would include event registrations, and any object that wasn't being GC'd.

    I think the crew has fixed the memory leak stuff, so the point might now be moot.

Sign In or Register to comment.