5

Resolved

RelayCommand and Memory Leak caused by CanExecuteChanged event

description

Hi Laurent,

CanExecuteChanged event keeps strong reference to Button which Command is bound to an instance of RelayCommand and it may leads to memory leak if we use long living view models with short living views. Maybe it would be better to use some kind of weak event listener. I made a fast try to use Silverlight Toolkit's WeakEventListener:

private EventHandler _canExecuteChanged;
public event EventHandler CanExecuteChanged {
add {
    WeakEventListener<EventHandler, object, EventArgs> weakEventListener = new WeakEventListener<EventHandler, object, EventArgs>(value);
    weakEventListener.OnEventAction = (h, s, e) => {
        h(s, e);
    };
    weakEventListener.OnDetachAction = (w) => {
        CanExecuteChanged -= w.OnEvent;
    };
}
remove {
    this._canExecuteChanged -= value;
}
}

and despite the fact that it might be not the perfect implementation, the problem with memory leak seems go away.

file attachments

comments

lbugnion wrote Jun 18, 2011 at 7:45 AM

Hi,

Yes this is a known issue. I am looking to fix that in V4.

thanks,
Laurent

mitkodi wrote Jun 19, 2011 at 8:34 AM

Thank you Laurent!

p.s.
Sorry for my post, I was asleep when I wrote it (especially the code fragment :-().

mitkodi wrote Jun 19, 2011 at 9:08 PM

... finally I managed to fix the problem for me, so I can safely wait for the next version of your awesome toolkit.

p.s.
In the RelayCommand I registered an attached dependency property and in CanExecuteChanged 'add' accessor I set it for 'value.Target' to 'value'. In this way the 'Target' (button) holds through this dependency property a strong reference to the delegate which it uses in subscription to CanExecuteChanged event and in the RelayCommand I can safely use only a weak reference to this delegate.

owoolgar wrote Aug 4, 2011 at 9:45 PM

Is there a workaround for this problem or an update as I too am incountering a leak that results in an application crash?

mitkodi wrote Aug 16, 2011 at 1:09 PM

Hi owoolgar,

If you want to try to workaround the problem before the version of MVVM Light Toolkit in which it will be fixed, fill free to try a little bit modified implementation of RelayCommand, attached to the post. It's entirely based on the Laurent's RelayCommand implementation to which I only added small changes:
- use a WeakReference to the target's CanExecuteChanged handler;
- inject strong reference to the target's CanExcecuteChanged handler into the target itself through an attached property.

andrevanderplas wrote Aug 19, 2011 at 4:50 PM

Hi mitkodi,

I tried your solution, but after calling the garbage collector to collect inside CompleteInitializePhoneApplication I didn't see the constructor (in which I write a debug message) being called.
Why do you think your solution works?
Weak references should be garbage collected after calling GC.Collect();

thanks,
Andre

andrevanderplas wrote Aug 19, 2011 at 4:51 PM

With the constructor not being called, I meant the constructor of several views I opened and navigated back upon.

mitkodi wrote Aug 19, 2011 at 9:53 PM

Hi Andre,

I wrote a small application for testing the solution of the problem (RelayCommandBindingMemoryLeak.Test.zip).

nico008 wrote Feb 17, 2012 at 7:47 AM

Hi there!

Any fully working fix for this memory leak?

I'm currently using a converter for using WeakEvent in RelayCommand.
Unfortunatly, I still have memory leaks : WeakReference, <GCHandle>, EventHandler, WeakEventListener<CanExecuteChangedWeakEventSource, ICommand, EventArgs> and more
When will it be included in official MvvmLight?

@Laurent : thanks for you great work!

Greetings from Belgium

and_rej wrote Aug 20, 2012 at 7:18 AM

Hi,

I was having an issue where in a unit test calling RaiseCanExecuteChanged had no effect. I expected the CanExecute method to fire as well a local event handler I had attached in my unit test. The RelayCommand in RelayCommand.zip fixed this. Why is this issue flagged a fixed?