Controls are like diapers: you don’t want a leaky one [Implementing the WeakEvent pattern on Silverlight with the WeakEventListener class]


This blog has moved to a new location and comments have been disabled.

All old posts, new posts, and comments can be found on The blog of dlaa.me.

See you there!

Comments (25)
  1. Delay's Blog says:

    In my last post, I explained how it was possible for "hidden" event handlers to introduce memory leaks

  2. LukeSw says:

    I thought that lines like:

    oldNotifyCollectionChanged.CollectionChanged -= OnCollectionChanged;

    prevents from leaking… it’s confusing 😛

  3. David Anson says:

    LukeSw,

    Detaching from the CollectionChanged event when ItemsSource is changed is certainly *necessary* to avoid leaking. But it is not *sufficient*. What if the control is removed from the visual tree while its ItemsSource is still set? Then there’s no indication that the control needs to unhook its event handler and leaks can result – unless the Weak Event pattern is employed.

  4. Delay's Blog says:

    We’ve just published the March 09 release of the Silverlight Toolkit and I bet there’s something in there

  5. LukeSw says:

    @Dealy

    So basically speaking, Weak Event pattern is needed so the programmer don’t have to remember to detach an even handler and/or ItemsSource property?

  6. David Anson says:

    LukeSw,

    The Weak Event pattern doesn’t exist so that programmers can be lazy – though it does make that a little easier. 🙂 Rather, it exists because there are certain circumstances where a perfectly valid program can get itself into a situation where a control it created is leaked and there’s virtually nothing the program itself can do to detect the problem or correct it. It’s for these unavoidable scenarios that the Weak Event pattern exists and is used.

  7. LukeSw says:

    Delay,

    I analyzed your sample and it seems that the WeakEvent pattern is helpful only in those situations when before discarding an object, a programmer forgot (or doesn’t know they must) to clear some properties (like ItemsSource) which attaches some handlers when being set.

    That’s really tricky.

    It’s a pity CLR doesn’t do that automatically… It would be better if delegates used weak references (I know that now it’s too late for their change/fix because of backward compatibility).

    quote: "What if the control is removed from the visual tree while its ItemsSource is still set? Then there’s no indication that the control needs to unhook its event handler and leaks can result"

    I don’t agree. Maybe I’ll provide not very common example, but what if the control is only temporarily removed from visual tree? It wouldn’t be useful if I had set ItemsSource whenever I want add back the control to the visual tree.

  8. David Anson says:

    LukeSw,

    You might be surprised that I agree with what you’re saying. 🙂 For cases like the scenario I describe, I agree that it would be nice if delegates used weak references. But I bet there are some very good reasons why they don’t – and besides, you’re right that it’s unlikely this will change now. Also, your example provides a good reason why – even if there *were* an event that was fired when a control left the visual tree – it would not be a good idea for a *Control* to null-out its ItemsSource. However, if such an event existed, it could be used by an *application* to do so – and then one might possibly argue that the Weak Event pattern was not strictly necessary. But that’s still putting a lot of burden on the application developer and it’s unreasonable to put that burden on ALL application developers. Therefore the Weak Event pattern exists and should be used by Controls when circumstances call for it.

  9. SharpGIS says:

    Is it just me, or is the reference only detached if the object raises an event?

    What if that event never gets raised? As far as I can tell in my sample code, then the references will never get released.

  10. David Anson says:

    SharpGIS,

    It’s not just you; if the event is never raised again, the code above won’t magically know to detach from the collection. If that’s a problem in your scenario, the most direct solution would seem to be to introduce some kind of periodic polling to the code above. However, the overhead of that seems like it could be more significant than the liklihood of a leak due to the event never firing again.

    If you have other ideas here, I’d love to hear them!

    PS – In researching this reply, I came across the following article I hadn’t previously seen: http://www.codeproject.com/KB/cs/WeakEvents.aspx. In it, Daniel Grunwald discusses a variety of different options in detail. However, it seems to me that none of the relevant options from Part 1 (excepting the WPF-specific one) address the issue you’re asking about, either.

    PS – Stay tuned to my blog for an update on some some very recent work in this area  (though not directly related to this problem). 🙂

  11. SharpGIS says:

    If you’re interested, here’s my sample code that demonstrates how badly this quickly can get: http://www.sharpgis.net/Samples/MemoryLeak.zip

    Just run the sample and watch the memory quickly grow every time it hits the simple timer_Tick method.

    I would love to hear your thoughts on how this leak could be remedied. I know I could get around it by implementing a Dispose() method, but it requires all developers to always remember to call this when they are done using the object.

  12. David Anson says:

    SharpGIS,

    Our comments may have passed each other in "the tubes" – did you happen to see my "February 20, 2010 11:02 PM" comment above with a link to further details and a possible non-Dispose workaround?

    At any rate, I don’t argue that this issue could be taken advantage of to create a significant leak in a contrived scenario – but do you think it’s likely to cause significant problems in more realistic scenarios as well?

  13. RScullard says:

    Sorry to comment so late, but this is still one of the best articles available on this topic!

    I just wanted to share a discovery I made, that SharpGIS's sample (above) contains a bug that causes it to leak not just WeakEventListener objects, but the actual objects that are supposed to be weakly referenced. This greatly increases the impact of the memory leak and defeats the whole point of using the WeakEventListener. It also illustrates the key point with WeakEventListener: You must make sure that your OnEventAction and OnDetachAction delegates are static!

    To fix the bug in the sample, change this line in Window1.xaml.cs:

      _weakEventListener.OnDetachAction = (weakEventListener) => property.PropertyChanged -= weakEventListener.OnEvent;

    to say:

      _weakEventListener.OnDetachAction = (weakEventListener) => value.PropertyChanged -= weakEventListener.OnEvent;

    Note that we are removing the reference to the "property" member, so our OnDetachAction is now static.

    Run it and you'll see the destructor on class B get called immediately. WeakEventListeners are still leaked, and the memory count still goes up, but the weakly-referenced instances are freed as intended.

  14. David Anson says:

    RScullard,

    Great info – thanks for following up! 🙂

  15. Jamil says:

    Guys what the license for using WeakEventListener class in my code ?

  16. David Anson says:

    Jamil,

    The code I post is under the Microsoft Public License (Ms-PL). There's a link to the full license from the "Information" section in the sidebar at the top of every post. The high-level summary is that you can use the code for pretty much anything you want. 🙂

  17. Kevin says:

    Hi Delay – I have a question regarding implementation of the above.

    I have a SL navigation view with the following code (in the Loaded handler):

    ComboBoxA.ItemsSource = null;

    ComboBoxA.ItemsSource = App.StaticObservableCollectionA;

    App.StaticObservableCollectionA is an ObservableCollection<Entity> – stored in App.xaml as a public static property, like so:

    public static ObservableCollection<Business_Role> StaticObservableCollectionA { get; set; }

    question one – this looks like a scenario that will cause my view to stay in memory even though i've navigated away from it.  right?

    question two – if the answer to question one is yes, how do I use this pattern to alleviate this problem?  for better or worse, i have this all over my app…

    the memory creeps and creeps as i move to and from views in this app.  I'd like to put a lid on it.  Thanks for your help.  

    Kevin

  18. David Anson says:

    Kevin,

    Yes, it seems like the reference to App.StaticObservableCollectionA will remain present even when the view isn't current. HOWEVER, the platform's ItemsControl class (upon which ComboBox is built) already does the right thing here to maintain weak references, so this shouldn't be the source of a leak for you (assuming this isn't a platform bug). That said, if you want to help be *sure* and cover all the bases, you should be able to set the ItemsSource to null in an Unloaded handler for your view (the counterpart to the Loaded handler you're already using). Alternatively, maybe do the attach/detach as part of the OnNavigatedTo/From process.

    You also might find it helpful to try to determine the source of the possible leak using the process I describe here: blogs.msdn.com/…/where-s-your-leak-at-using-windbg-sos-and-gcroot-to-diagnose-a-net-memory-leak.aspx

    Hope this helps!

  19. kmkuntz says:

    Thanks for the response.  I'll set the source to null upon leaving the view for sanity's sake, and will be checking out windbg immediately.

    one last one – what about a simple, completely self-contained button click handler defined on my view for a button on my view?  does that need to be unregistered?  If the button is defined on the view it would seem that it should have the same lifetime as the view, and therefore shouldn't be a problem.  it will die when the view goes, regardless of the handler.

    is this correct?  or should ALL handlers be unregistered?  

    Thanks for your time.

    Kevin

  20. David Anson says:

    Kevin,

    Your understanding is correct: if the thing subscribing to the event has the same lifetime as the thing that exposes the event, there shouldn't be a leak concern because both instances will become garbage at the same time and the fact that one references the other won't matter. The big concern is when there's a long-lived (i.e., for the life of the application) collection with one of these "backward references" to an individual page instance – especially if that new instances of that page are created regularly! 🙂

  21. kmkuntz says:

    Thanks for your help David.  That clears up a lot.

  22. kmkuntz says:

    Using WinDbg I was able to locate the source of my memory leak (good!).  Turns out it is caused by the use of a PanelDragDropTarget (bad!), as discussed here:  silverlight.codeplex.com/…/7356.

    Not sure how to resolve this one other than removing the panel, which will also remove functionality that our users are used to having.

    Thanks for the advice.  

  23. David Anson says:

    Kevin,

    Sorry about the trouble! However, from the comments of the issue you link to, it looks like folks may have been able to fix the leak. If you have time to experiment, maybe that would resolve the leak without requiring you to abandon the panel?

  24. kmkuntz says:

    Yes sir that's what I'm doing – trying to figure out how to use their patched DragDropTarget.  Looks like I'll have to grab the entire toolkit source unless there's something I'm missing (which is entirely likely).  

    I appreciate all the help David.  This would have been quite a bit more difficult without the back and forth.

    Kevin

Comments are closed.