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
Android: MvxBindableListItemView instances leaking #17
Comments
Thanks Blewzman Have you got any more info on what you are seeing? I'm not 100% sure what you are asking about. A LinearLayout is Is there a complete project I can see that illustrates the issue you are Thanks Stuart On 27 June 2012 17:48, Blewzman <
|
Hi Stuart, thanks for the quick response. I see my description of the issue is lacking. In my app, the bindable LinearLayout of your framework is bound (through ItemsSource) to a collection that implements INotifyCollectionChanged. During the lifetime of the app, this collection will grow and shrink. The LinearLayout reflects this perfectly - there is always the correct number of child-views visible and each of these childviews is bound correctly to the item of the collection. However, when the collection changes and the LinearLayout is refilled, the old views are removed from the LinearLayout but they remain referenced by something and this is what leaks. So even if the number of items in the source collection only varies from 0-10, i will end up with hundreds of MvxBindableListItemViews. Full Garbage Collection eventually kicks in every two seconds until it blows up. If I remove the binding of the child-views (a simple binding of some string to the Text-Property) there are no leaks. So I suspect the binding is not disposed of when the child view is disposed of (if it is at all - during refill of the LinearLayout) Activities do not play a role in this scenario, there is always the same activity displayed. There is currently no complete project but I'm working on that. P.S. In my original post the layout got screwed up because of the XML tags. I replaced "<" by "-", I hope it makes sense for the moment. Blewzman |
That sounds plausible Thanks for the longer explanation. Currently on a train, but just prototyped some code. Will send when I get home (tethering arrives on my lumia any day now...) Thanks again. If this is the bug you suggest then I suspect it also Stuart Sent from my Windows Phone thanks for the quick response. I see my description of the issue is However, when the collection changes and the LinearLayout is refilled, If I remove the binding of the child-views (a simple binding of some Activities do not play a role in this scenario, there is always the There is currently no complete project but I'm working on that. Reply to this email directly or view it on GitHub: |
Out of interest, why are you using a linearlayout rather than a list? Sent from my Windows Phone thanks for the quick response. I see my description of the issue is However, when the collection changes and the LinearLayout is refilled, If I remove the binding of the child-views (a simple binding of some Activities do not play a role in this scenario, there is always the There is currently no complete project but I'm working on that. Reply to this email directly or view it on GitHub: |
Great, thanks. I need to place the child views horizontally, I tried with list but that was only getting me a vertical layout (with bars between boundaries, which is fine for other activities in my app). |
Here's an attempt at a Dispose() implementation. Not got a full repro case here so can't test this properly It looks ok... but definitely needs some testing - I don't even know if the Dispose() gets correctly called right now. Stuart
|
Thanx! I will add your implementation of Dispose to my checkout of MvvmCross tomorrow and see whether it is called at all and whether it solves the problem. Daniel |
Hi Stuart, I did a quick test. First I just implemented the Dispose method on MvxBindableListItemView. Unfortunately, it is not called when the MvxBindableListItemView is removed from a ViewGroup (LinearLayout in this case). So a call to RemoveAllViews obviously will not dispose the children. I then did a reimplementation of MvxBindableLinearLayoutExtensions.Refill:
The problem with this solution is obviously that it is specifically for the LinearLayout only and that code is likely to be repeated - not very nice. I have not tested a binding on the childview yet, so I don't know yet whether disposing the child view solves the leaking problem. I will test that later today. Daniel |
Thanks for the info. Just to check: your Refill rewrite - did it actually stop the leak? I'm confident the bound views within a child activity should get cleared - because the Activity clears all bindings using However, for the situation you are currently using, then the list of bound views inside the activity could get rather large - so it will chew memory as you've discovered. The problem of repeating code is a general one in Mvx - never before have I wanted mixins or multiple inheritance quite so much in C#! As an alternative to your Refill rewrite another solution might be to use a
with the list item code rewritten to include:
Well found and diagnose on this problem so far. I've only used the LinearLayout binding for a very small amount of work so far - and that was in quite a static situation - for most other situations I've used lists and there the cell reuse helps with limit the memory consumption - although I'd be quite happy to add the ChildViewRemoved code there too (after some testing!) When we get somewhere we're both vaguely happy with I'll definitely push some fixes for this to the tree - thanks for the investigation on this! Stuart |
I take that last post back. For ListView we wouldn't want to do the exact same code during the ... would definitely needs testing on a few different android versions just to make sure I understand it :) |
Hi Stuart, good news: Calling Dispose on MvxBindableListItemView stops the leak! I only tested my Refill-rewrite and not the ChildViewRemoved-Handler. I would like to contribute to the project, particularly in solvin this issue. One question: How crucial is it to utilize an Adapter class? I mean, does the code at some place depend on the LinearLayout using an Adapter internally? I have written a bindable LinearLayout-like class in the past for the Compact Framework, it handles INotifyCollectionChanged-events on a finer level (just replacing the views where neccessary and not refilling the entire Layout). It handles everything itself, does not depend on any Adapter and can be bound to objects implementing IEnumerable or INotifyCollectionChanged. |
Not crucial, but there is code that can be reused there. Thinking out loud... The reason I used an adapter originally was to allow code sharing between One reason I used such limited INotifyCollectionChanged support was because Another reason I used such limited actions was because the default Android If you wanted to write a linearlayout that took note of the actions (or You could probably do this while still using the existing adapter if you Stuart P.S. Sorry for the rambling answer and for my MvxFarTooLongClassNames! |
I have a quick attempt at a bindable items-View that should be reusable for every kind of ViewGroup-Layout. I post it here, I hope the formatting doesn't get screwed up:
|
Specialization on e.g. LinearLayout can then be done by:
|
I just saw that the code in the last posting will not compile because LinearLayout does not have a public parameterless constructor. Nevermind, we can always take the generic parameter out of the code and put a protected abstract method "CreateItemsViewGroup" (or something like that) into MvxBindableItemsView. |
Yeah - thanks - general approach looks like the right sort of thing I guess Move isn't too hard to do either... I also need to think and/or readup about calling Dispose on Android Views - I'm focussing on the day job right now, but wondering about carrying on
I think a third way might be the best approach... something that separates Thanks again for giving me something fun to think about today :) |
I wonder if this would work with a map view and pushpins... On 28 June 2012 16:02, Blewzman <
|
If we really want to keep the adapter, why not let the adapter maintain a list of the Views it created, so it establishes its own 1:1 relationship between sourceObjects and Views and can dispose the views accordingly. However, I somehow feel that the method
suggests that somehow the user of the Adapter is responsible for maintaining the views (which is exactly what a ViewGroup does), so there's not much separation of concern here.... Your idea of a completely different class to handle is good - basically what we need here is some kind of projection from a list of some type T to a list of type View - like the LINQ Select-method on IEnumerable, just not on IEnumerable but on INotifyCollectionChanged / ObservableCollection. Once we have projected the source collection s to a View-collection c, MvxBindableItemsView could easily subscribe to INotifyCollectionChanged on c and then just insert and remove the views as they come. |
I think this is exactly right. It's exactly what an Android ListView does. The LinearLayout (or MvxBindableItemsView) will maintain a 1:1 list, but In terms of separation of concerns, it's not perfect, but it is the Android Will have a proper play when I get a chance... Stuart |
I hacked again to produce the files: https://gist.github.com/3012791 These 3 files compile... but I really need to step back and build a test harness which inserts, removes, replaces and moves within a list :) Beyond this first code:
Thanks again for the original problem report and for the further thought and investigation you've put in :) |
Definitely, eventually for every ViewGroup derived type there would have to be a type that inherits from the generic type (like in "class MvxBindableLinearLayoutEx : MvxBindableItemsView").
Just had a quick look into the docs - how can ListView possibly react to Adapter changes on a level as fine as we are just trying to - after all, there's only "NotifyDataSetChanged" and that doesn't give too much information. Given that ListView derives from ViewGroup, I wonder if ListView could be used without any Adapter at all, just by our generic approach. Of course, the user of the ListView should not be able to set an Adapter manually then since this would mess up all logic. |
I think that could be done - but rewriting the listview might be a future As a first step, I just want to focus on getting both the listview and the Stuart |
Alright. If you need some coding done by me let me know. Maybe I do my own experiments with the generic approach and see how it works with the ListView and whether it is promising at all. I'll file another issue later for which I have already found a solution. Daniel |
Hi Daniel I've pushed a version which I hope doesn't leak.... but I don't really have a good test case to verify it. The version I've gone for uses an adapter - my reason for this is so that it can share the core data source binding code with BindableListView, BindableAutoComplete and BindableSpinner. Currently, I've not made the LinearLayout inherit from a generic base class - just because I'm out of time right now and because I personally don't have a use for it right now. However, I'd be very happy for you to contribute a generic base class implementation if you need one - but I would prefer that to use the adapter if it can... happy to discuss this if you're interested (it might be easier to discuss "live" on http://jabbr.net/#/rooms/mvvmcross rather than on here) Hope the memory fix works for you - thanks again for taking the time to report the bug and for all your assistance in investigating it Stuart |
Hi Stuart, thanks for the updates. I'd be happy to contribute some code to your project. I also see that the ListView and some other Views will not do anything useful without an adapter. The bad thing about it is that it leads to some repetitive code, and all the bindable views hide the base class implementation of the Adapter by the 'new' keyword, which is not really satisfying either. Then, we have ViewGroups that could be bindable but do not have any adapter at all (such as LinearLayout). Very complicated. I just realized how I love the WPF/Silverlight approach of binding any type of composite view (Panel) to any list by the use of ItemsControl. I will think about a solution. |
Hi Stuart, me again. I haven't yet had the time to come up with a re-implementation and abstraction of the bindable lists, but I investigated another memory leak. Suppose we have a ViewModel:
which exposes a list of items of type DebugViewModelItem:
These are bound by the following Layouts: DebugLayout.xaml:
DebugItemLayout.xaml
Load these into the following Activity:
The activity loads DebugLayout, and after 2 minutes opens another empty activity not given here and finishes itself. Here's what happens: The activity is bound correctly and "EventHandler added" is output 100 times. We would now expect "EventHandler removed." to be output another 100 times, this does not happen. Unless the ViewModel goes out of scope, it keeps a reference to the binding (through the PropertyChangedEventHandler field). I found out that the bindings on the items are not stored in the tag-property of the activity but elsewehere (I think in the BindableLinearLayout) and they aren't cleared. An approach to implement or override Dispose everywhere does not work because Android does not bother disposing all the child-Views when an Activity is destroyed. I finally added some code in MvxBindingActivityView: In MvxBindingActivityView.ClearBindings(View view), append
This is a rather brute force approach, it works for me but I don't know if this does always what the user intended. In MvxBindingActivityView.ClearBoundViews, replace all code with
I also wonder what happens when a BindableLayout instance is not in _boundViews but has bindings itself (is that even possible?). What do you think ? Best regards, |
Thanks Daniel. From what you are saying, the bound controls are somehow not getting added to _boundViews - whichs sounds strange (sounds like you've found another bug!)
That's correct - the binding should be stored in the tag of the bound view (whatever was inflated) - but that view should then get added to _boundViews collection - so the bindings should get cleared up either when OnDestroy() or Dispose() happens - whichever happens first! I'll run the code.... ... and .... Good find - that's a serious leak! The bug is in:
When the second inflate method is used, then the Will fix with:
Sorry for the bug Thanks HUGELY for the report! Stuart |
Hi Stuart, thanks for the fix. I found another potential leak. Suppose we have a ObservableCollection which is bound to the ItemsSource property of a MvxBindableListView. Upon disposal of the Binding, the ItemsView property is not set to null, meaning that the ObservableCollection will keep a NotifyCollectionChangedEventHandler-delegate to the MvxBindableListView (more specifically to its Adapter). The code in my last post can be used to reproduce this, with some changes: Create a ObservableCollection-derived class for debug output:
Change DebugViewModel:
If you run this code, "NotifyCollectionChangedEventHandler added" will be output, but "NotifyCollectionChangedEventHandler removed" won't when the activity is destroyed. I tried to find out how this is handled in WPF but couldn't find documentation on how the target is updated when the binding is disposed. I did a quick workaround:
This is then registered with
The code has flaws - for example I don't know whether using MvxPropertyInfoTargetBinding as baseBinding is always justified, but it works for now. Daniel |
Interesting (again!) Would need to think on this one. For "static pages" (normal activities) I guess there wouldn't be a leak as both View and ViewModel should disappear off together into the deadpool, taking the collection with them, but I can imagine there might be a leak for pages which load/switch panels dynamically into place. WPF might be able to cheat - it might be able to hook into the destroy event for the control to unhook all event listeners. I guess we could similary just hook into Dispose() on the ListView - but I'm not confident on when that gets called. Will need to think about this one. I guess instead of the "setting to null" solution we could add some sort of Definitely need to think on this one... Thanks yet again! Stuart |
I think I have a good solution to this problem. Are you familiar with weak events? I wrote a simple but working implementation of a WeakEventManager (a concept present in WPF too) that would handle all the memory leaks coming from un-unsubscribed events. |
I'm a bit familiar with Weak references - I've used them indirectly in MvvmLight (it's messenger is weak reference based, probably for very similar reasons) - http://bit.ly/PAmzau I considered using WeakRefs in the binding originally - but shied away from it because I thought we could keep control without them. Is your prototype up anywhere? Would be interested to play with it Before using them, I think we should discuss it through with users... e.g. I might asking @lbugnion about what negatives (if any) they've caused in mvvmlight. Always interesting :) |
Its not up anywhere, having just written it. Can I upload it anywhere here? I'm not too familiar with Git or Github though... |
Quickest way is probably the new http://windows.github.com/ tool. I'm still mainly using http://gitextensions.googlecode.com/. A 2 minute guide to how I work is:
Given how git seems to be winning, it's a skill worth learning (IMO) If not.... if that's too complicated, then you can also copy a small number of files using http://gist.github.com Hope that helps |
Did it! It's in Library/WeakEventManager https://github.com/Blewzman/MvvmCross/tree/b4030a7fae01eba8bdd278d9ba59f60e463e260a/Library/WeakEventManager Let me know what you think of it. |
Welcome to git ;) If you ever need free private git repositories, then try bitbucket.org. I think this is definitely an interesting idea... And the fact that MvvmLight uses a similar approach (in a slightly different scenario) encourages me. Are you thinking that all INotifyPropertyChanged and INotifyCollectionChanged subscriptions should go through weak ref mechanisms? Or should just some of them? I guess it would be possible to make this configurable per-use - but I have a feeling that this should go "all in" if it happens as that would make the architecture "cleaner" and "simpler" I'm also wondering about whether to do both this and the null change - I can see cases where users would expect that disposing the binding without destroying the view - and in that case they would want the events to stop... Always interesting! |
I really think too that this should go "all in" - mainly because it is not at all obvious where to use weak refs and where not. Best thing, the 'set to null' feature wouldn't even be necessary then, since the INotifyCollectionChanged subscription would eventually go away too. I don't think disposing the binding without disposing the view is such a reasonable scenario (although possible I think). Given that we ideally design our views in declarative ways (xaml/axml), it seems rather strange to declare a view and the bindings in xaml and then change that programmatically by disposing the binding. |
I'm still thinking about this.... My current thoughts are:
As well as the "set to null" idea I've also wondered about adding some sort of disposable wrapper around INotifyCollectionChanged and INotifyPropertyChanged within in target binding code - but I'm really not sure about which is the lesser evil! The good news is this is all about "edge cases" - so there's no huge time pressure on this (phew!) Stuart |
The cases are not that "edgy". For example I found out that the BindableListView does not have the workaround you did for the BindableLinearLayout, and it wouldn't even be possible because it does not raise the OnChildViewRemoved event. So there's no chance of BindableListView to not leak as far as I can tell. |
Thanks again I had to go back and remind myself what you were talking about - it's been a while since that workaround :) However, I don't think that the original problem effects the ListView. I don't believe the ListView needs an OnChildViewRemoved as it doesn't maintain one view per list item - instead it virtualises the UI and maintains a reusable set of views (convertView's) which it uses for all the list items. As you add and remove children from the list, then (unlike the linearlayout) the set of child views in the ListView should not grow too large - instead it should just be the right number to fill the screen. For a "normal" activity inflated from "normal" XML then all of the child bindings should get disposed when the activity disappears (after the fix for So as I currently understand it, the only open problem here is with the leak caused by the INotifyCollectionChanged subscription. This is the case where:
As far as I can see, this shouldn't occur for most apps where the observablecollection should get garbage collected at the same time as the ViewModel and the View... because most observablecollections will be owned by the ViewModel. That's why I think it's "edgy". This will only occur where the observablecollection lifetime exists beyond the lifetime of the ViewModel, or where listviews (panels?) are dynamically created and removed within an activity without the activity itself ever being destroyed. Please be assured that when I say "edgy" I don't mean that I won't fix it... I just mean that I don't think this leak occurs in typical, normal use - e.g. I don't think it occurs in any of the 6 samples. At least, from what the debugger is telling me I think that's the case... |
I think I'm reaching my conclusion on this point. Here's what I'd like to propose....
I prefer to do this actively rather than waiting for WeakReferences as I personally feel this is the correct way to do it and I can't think of any situation where we can't Dispose the binding. I feel like we shouldn't be relying on WeakReferences to clear up my mistakes. I've tested this on Droid and it seems to clear up the objects and didn't have any adverse effects on any of the samples. Will test on Touch "soon"
Does this sit OK with you? Do you think I'm making a big mistake in including the SetToNull code? Is there some bug you are worried that I am causing. Thanks again for your input - it really is valuable! Stuart |
Hi Stuart, I don't really know yet whether setting the property to null is always a good thing to do - the setter might throw an ArgumentNullException when null is an invalid value for the property, you might catch it but performance would decrease (which is not a big consideration now for me). I think it would be bad style to provoke and catch such potential exceptions as part of the normal control flow and there's no way to determine valid values for a property. Worse, null might have some semantics for certain properties, and the setter will not be able to tell whether is has been passed null because of normal operation or disposal of the binding. I did the set-to-null-BindingFactory workaround for a specific propertyName ("ItemsSource"), so I could control the effects to a certain extent. Making it a default, I really don't know.
I don't think this is a mistake per se. We don't know anything about the Views or the ViewModels, we don't know about their internal workings, we don't know whether null is a valid value for a target property. But if you don't know anything, how can you tell whether a view will leak if you don't set it's xyz-property to null? The ItemsSource property is just a special case that we now know about.
This is a good idea - do you mean a compiler directive or something that can be set dynamically at runtime? Would you say it's reasonable to switch to the vNext branch right now ? Daniel |
Thanks again, Daniel I like the thinking around "we don't know whether null is a valid value for a target property" We could add an attribute - something like:
This could then be used as:
and:
On vNext, I do need to check it against the latest MonoTouch code (6.0 just released) but overall yes I think it is usable and stable - the main problem for users is the required hacking to get the tooling working perfectly :/ The main code changes for apps are:
There's no hurry for anyone to switch over - but I do very strongly think that the PCL approach is the future. Some people will no doubt hate the PCL approach - in which case I will keep vOld there for them to use - but as a Resharper user, it's heaven for me! Will get lots done this weekend - back to work now :) Stuart |
Attributes are good, but what about the cases where a user would want to set an attribute on a method in a closed source assembly ? Maybe this should be set directly in XAML by the notion of a fallback value as in http://msdn.microsoft.com/en-us/library/system.windows.data.bindingbase.fallbackvalue.aspx |
That's definitely an edge case ... but I guess they could always inherit from the control... or if absolutely necessary they could use a custom binding The code does already have FallbackValues - they're stored and used in the MvxFullBinding class. They could be used, but they're kind of there for a different purpose - they're there for genuinely missing sources. Thanks again Back to work! |
So it all would come down to whether we can think of a disposed binding as some kind of missing source, wouldn't it ? |
Not really - the arguments against null also stands for the arguments against FallbackValue - for objects there's no real fallback value available in xml or json - so default(T) gets used - which is null... I could push this as far as an attribute which somehow indicated a method to call on unbinding - e.g.
or this info could even go into a new field in the binding JSON?! But for now.... I'll test out the simpler attribute first - want to make sure it doesn't do anything nasty when e.g. ItemsSource and SelectedItem are both bound (e.g. does a SelectedItemChange event fire if I dispose ItemsSource before SelectedItem?) It's good to be busy :) But I really need to get some unit tests on vNext! Stuart |
…listviews - this version is mostly tested... but there's potentially more still to do!
Pushed an attribute based fix for this... but now really want to do weakReferencedEvents too - especially as I understand more about UIKit on MonoTouch |
I will close this.... when I implement some weak reference stuff (maybe as part of #61) |
Was talking about this again yesterday. WeakReferences will happen - but possibly only at the final layer of binding to the UI |
Great to hear. I was removing some of the code I added to my fork of MvvmCross to get the leaks out, now hat we have SetToNullAfterBindingAttribute. I was able to restore all the original code except for two places in MvxBindingActivityView where the original code still leaks. I was able to stop the leakage by inserting
at the end of MvxBindingActivityView.ClearBindings(View view) and replacing the body of MvxBindingActivityView.ClearBoundViews() by
This essentially does a recursive cleanup and kills the leaks. I tried but I can't see where the original, non-recursive cleanup fails. |
Thanks for bearing with me.... I'm still seriously thinking about adding the weakeventmanager too. ie. adding it as well as all the other code It's grown on me over the last 8 months! 8 months?! Sorry - I'm slow sometimes! Stuart |
Hi Daniel Sometimes it just takes time for me to get things.... So please do exactly as you have one here - post me suggestions and then leave me alone for a few months... I'll get there eventually :) The current v3 prototype code is https://github.com/slodge/MvvmCross/tree/vNextDialog/Cirrious/Cirrious.MvvmCross.Binding/WeakSubscription It has some accompanying extension methods in https://github.com/slodge/MvvmCross/blob/vNextDialog/Cirrious/Cirrious.MvvmCross.Binding/ExtensionMethods/MvxWeakSubscriptionExtensionMethods.cs And it's used like in https://github.com/slodge/MvvmCross/blob/vNextDialog/Cirrious/Cirrious.MvvmCross.Binding.Droid/Views/MvxAdapter.cs It's loosely based on your weakreferencemanager code - thanks :) It's very early code yet - will add some test harness and do some more testing tomorrow. I will get there eventually Thanks Stuart |
I'm going to be very brave and close this issue. Daniel/Blewz - no idea who you are... but I'm very grateful for your patience and skill on exploring this area with me. It feels like the Weak references in v3 are awesome. I'm sure we'll hit subtle bugs with them - but overall they make me very happy. Thanks! |
Fixed crash when initial View is marked as ActivePanel
Resolve merge conflicts
Hello,
consider the following layout:
-cirrious.mvvmcross.binding.android.views.MvxBindableLinearLayout
android:layout_width="fill_parent"
android:layout_height="fill_parent"
android:orientation="horizontal"
local:MvxItemTemplate="@layout/itemlayout"
local:MvxBind="{'ItemsSource':{'Path':'Items'}}" />
where "Items" will be an object implementing INotifyCollectionChanged and "itemlayout" is
-FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:local="http://schemas.android.com/apk/res/MyApp"
android:layout_width="44dp"
android:layout_height="wrap_content">
-TextView
android:background="#ffdddddd"
android:layout_width="40dp"
android:layout_height="28dp"
android:textColor="#ff000000"
android:gravity="center"
local:MvxBind="{'Text':{'Path':'ID'}}/>
-/FrameLayout-
then I found the created instances of MvxBindableListItemView (and the contained FrameLayout and TextView) to be leaking. Moreover, MvxJavaContainer instances leak in roughly the same number.
There is no leaking when the itemlayout is not bound.
What I found out using the Dalvik Debug Monitor and the Eclipse Memory Analyzer ist that all MvxBindableListItemView, FrameLayout and TextView instances are marked as residing on the native stack, they mainly hold circular references to another (with some ColorDrawable instances in between). I couldn't really find any Java-Class holding to all that instances so I assume the references are kept alive by the binding on the .net-side (with MvxJavaContainer possibly bridging the two sides).
The text was updated successfully, but these errors were encountered: