RelayCommand: memory leak vs. behavior bug

[Josh Smith]

Hey all,

Someone just pointed out a bug in RelayCommand.  

http://mvvmfoundation.codeplex.com/Thread/View.aspx?ThreadId=68127#Post231976

The fact that the CanExecuteChanged event internally uses CommandManager.RequerySuggested as the "backing store" for the event exists for two reasons.  It ensures that all RelayCommands are queried by WPF when the built-in commands are queried to see if they can execute.  And, also, since RequerySuggested is a "weak event" the listeners are not rooted, which helps prevent the well-known memory leak in WPF around commands not being unhooked by command sources (button, etc.).

It turns out that RelayCommand's CanExecuteChanged event handlers can be GC'd at any time.  Yikes!  Clearly this needs to be fixed, but it's not clear to me what the proper solution is.  If I don't use RequerySuggested, I don't get the nice built-in CanExecute querying and I opt in for WPF's memory leak issue.  If I do use RequerySuggested, then RelayCommands can lose their CanExecuteChanged behavior after a GC or two.

I've looked through the code in WPF related to this, but don't see what the built-in commands are doing special to prevent this bug from occurring...

Any ideas?

Thanks,

Josh

[Karl Shifflett]

Josh,

 

I’ve never lost the functionality of RequerySuggested in any UI when following the View, ViewModel pattern we use, even if I run GC.Collect.

 

Karl

[Karl Shifflett]

Josh,

 

I’ve attached a sample project that uses RelayCommand.  Rename extension to .zip.

 

I has a button to cause a GC.Collect

 

I’ve pressed it 50 times and no disconnect occurs in the other Button that uses RequerySuggested to test if the TextBox is not empty.

 

Your thoughts?

 

Karl

[Bill Kempf]

Karl,

You've done something different from the unit test. Your delegate is
to a member, while the unit test uses a lambda. I don't see how that
would make a difference in behavior, though. The reference is going
in the wrong direction to root the delegate here, I would think? It's
late, and I've not gotten much sleep lately, so my mind is reeling
trying to puzzle this one through. But I suspect there's a difference
here somewhere that's significant. I'm willing to bet that the typical
usage is fine here (like you've demonstrated), but some atypical usage
can result in this behavior, and that explains why no one's reported
an issue yet. IOW, I don't think there's a bug in RelayCommand or WPF,
but rather in the calling code. I'm sure that after reading this,
someone else will explain what it is, and I'll feel stupid for not
being able to puzzle it through tonight, but I'm just to exhausted to
care ;).

--
Quidquid latine dictum sit, altum sonatur.
- Whatever is said in Latin sounds profound.

War is peace. Freedom is slavery. Bugs are features.

[Karl Shifflett]

Bill,

Thanks for your feedback.

I'll change the application over to use a Lambda and see if that changes the
landscape.

I quit using Lambda's for relay command a while back because they sit around
in memory. Not sure why they seem to float around in the heap.

Karl

[Karl Shifflett]

Bill & Josh,

Still works with Lambda's.

[Bill Kempf]

But does the lambda call a member? In the unit test provided (I didn't
run it, but have to assume it fails as advertised) the lambda is
extremely "barren":

() => canCancelCalled = true

[Karl Shifflett]

Yes, my lambda does call a member.

I see what you're implying now; barren for sure.

[Andrew]

You can probably reproduce the issue without lambdas or anonymous methods. Just create a class 'A' that hooks the event. Create an instance of A and have it hook the event. Don't hold a reference to that instance, do a gc collect and there should be no listeners. It's not a bug - it's just that since nothing is rooting the instance of A (the event uses a weakreference to avoid rooting the listener and nothing else is rooting the instance) it will be collected.

-Andrew

[Mark Smith]

Something like this:

    public partial class Window1

    {

        public class A

        {

            public A()

            {

                CommandManager.RequerySuggested += CommandManager_RequerySuggested;

            }

            void CommandManager_RequerySuggested(object sender, EventArgs e)

            {

            }

        }

        WeakReference weakRefA = new WeakReference(new A());

        public Window1()

        {

            InitializeComponent();

            tbCollected.Text = "IsAlive = " + weakRefA.IsAlive;

        }

        private void OnDoGC(object sender, RoutedEventArgs e)

        {

            GC.Collect();

            tbCollected.Text = "IsAlive = " + weakRefA.IsAlive;

        }

    }

It will be alive until GC collection occurs, then "A" will go away.  I would assume if the RelayCommand didn't have anything rooting it, it would get collected as well.

mark

[Mark Smith]

It's late and I'm tired, but in most cases don't you hold a reference to your RelayCommand?  I.e.:

public class MyViewModel

{

    public ICommand SomeCommand = new RelayCommand(....);

}

In this case, it's rooted by the MyViewModel instance and so it shouldn't get collected.. I might not be thinking clearly but it seems like that's ok..

mark

[Eric Burke]

I'm able to reproduce the issue even if I create a member which is the RelayCommand (instead of putting it on the stack).

One thing I noticed while digging thru Reflector is that the RequerySuggested comes at Background priority, which is the same priority that his DoEvents() posts at.  However, changing DoEvents to use ContextIdle doesn't change the behavior.

I'm going to muck with this some more tomorrow.

---

From: Andrew <webfl...@yahoo.com>
To: "wpf-di...@googlegroups.com" <wpf-di...@googlegroups.com>
Sent: Monday, September 7, 2009 11:08:18 PM

[Josh Smith]

Thanks for the input so far, guys.  I'm still puzzled over this one.  It's still not clear to me what the actual problem and fix might be.  Should the way that RelayCommand is used be considered a bug, or should RelayCommand "act properly" regardless of how the can-execute logic is provided?  Argh...I need some sleep. :)

Thanks,

Josh

[Philipp Sumi]

Josh,

 

I think that's a common issue when dealing with weak events - the WeakEvent implementation I'm using here even explicitly tests for closures because they are being garbage collected right away

(fyi - it's this one here: http://www.codeproject.com/KB/cs/WeakEvents.aspx).

 

Regarding Andrew's scenario, I'd say this is perfectly legal, anyway - after all, it's the idea behind having weak events that listener can in fact be garbage collected. Regarding closures, on the other hand, it is indeed a limitation.

We might be able to test for this with a bit of reflection, however, as the closures are decorated with a CompilerGeneratedAttribute:

 

//directly taken from Daniel's WeakEventImplementation

MethodInfo method = GetMethodFromCaller();

if (method.DeclaringType.GetCustomAttributes(typeof (CompilerGeneratedAttribute), false).Length != 0)

{

  throw new ArgumentException("Cannot create weak event to anonymous method with closure.");

}

 

...I haven't dug deep on this yet, but could have a look at a test for the command later this afternoon.

 

Cheers,

Philipp

[Mike Brown]

If I'm understanding correctly it's not the RC that's getting collected, it's the delegate to requerysuggested. and because requerysuggested is using a weak reference (not to the delegate itself, remember the WeakEvent is implemented as a very thin object whose sole purpose is to invoke the delegate it is holding). So the problem is that the only guy who has a reference to the CanExecuteChanged delegate is a weak reference that can be taken out at any time.

 

I think the suggestion of keeping a reference to the delegate will fix that, but as you said there is the memory leak dilemma. It's too late for me to try to investigate this one but tune in tomorrow for the thrilling conclusion.

[Jaime Rodriguez]

From an old internal thread w/ Varsha, the lead on Tree Services.
“RoutedCommands don’t have their own CanExecuteChanged handler storage. They use CommandManager.RequerySuggested instead” ….  So I don’t think RelayCommand is too far off..  

 

------------ now back to your scenario ------------------

The scenario where nothing holds a reference to a ViewModel exposing a command, is not common …  and the patterns I use for commands is delayed instantiation and  single instance, so my commands are always members of the ViewModel exposing them; so I use some thing similar to RelayCommand.  

The way I see it, the choices  were:

1.- A very likely leak on a common scenario,  if I created a simple (member store) backing up CanExecuteChanged… and then forgot to -= .

2.- The issue you describe in a  very unlikely scenario where nothing holds a reference to a VM exposing a Command.. and the CanExecute code is collected.

3.- Adding complexity if I created a store that was weak in my command…  and then possibly having another non-deterministic scenario not much different from the one I was trying to solve in #2...

 

In the end, I opted for #2 because of the odds; I knew how easy it is for me to leak memory.. and could not afford to do #1.   I think the scenario where nothing references the VM that exposes the command (and references it via member) was very unlikely..  So far, it is working for me. NOT claiming that to be the answer; just re-visiting the trade-off ( of a leak)..

 

The WPF team created pseudo-code for DelegateCommand once … when we considered adding it to WPF 4.  The pseudocode had CanExecuteChanged in a backed store..  but I can’t see if that was a weak reference or not ( it is not in the pseudo code I looked at)  so if it helps, the WPF team was going towards 1 or 3 above (from pseudo I can’t tell if reference was weak or not)..

 

Sorry if this does not help.   Looking forward to better answer;  and most important looking for others to high light valid scenarios where this issue does happen; I can read the example below, but that sounds unlikely, since you have a class that is not referenced;  in MVVM, the VM is always referenced (else nothing works)..  

[Peter O'Hanlon]

To be honest, I'm having a real problem seeing where this issue will occur. I can see what he thinks the potential problem is, but I can't see any valid situations where it is likely.

--
Peter O'Hanlon

[Mark Smith]

Ah yes, I see now. I completely misunderstood when I threw off a
reply late last night. I see now it's the delegate being collected,
which makes perfect sense.

Looking in Reflector, it looks like all three implementations of
ICommandSource hold the event handler itself in DP storage using
UncommonField<T>:

private static readonly UncommonField<EventHandler>
CanExecuteChangedHandler;

private void HookCommand(ICommand command)
{
EventHandler handler = new EventHandler
(OnCanExecuteChanged);
CanExecuteChangedHandler.SetValue(this, handler);
command.CanExecuteChanged += handler;
UpdateCanExecute();
}

private void UnhookCommand(ICommand command)
{
EventHandler handler = CanExecuteChangedHandler.GetValue
(this);
if (handler != null)
{
command.CanExecuteChanged -= handler;
CanExecuteChangedHandler.ClearValue(this);
}
UpdateCanExecute();
}

Keeping a reference to your delegate in RelayCommand would certainly
fix the issue.. I need some coffee to think through this some more..

mark

[Mark Smith]

> If I'm understanding correctly it's not the RC that's getting
> collected, it's the delegate to requerysuggested. and because
> requerysuggested is using a weak reference (not to the delegate
> itself, remember the WeakEvent is implemented as a very thin object
> whose sole purpose is to invoke the delegate it is holding). So the
> problem is that the only guy who has a reference to the
> CanExecuteChanged delegate is a weak reference that can be taken out
> at any time.

And to further draw out, it only happens to user handlers which are
trying to watch the CanExecute state - WPF controls maintain their own
reference to the handler so Button, MenuItem and Hyperlink do not have
this problem - even with RelayCommand. It doesn't appear that
RoutedCommand does anything to stop this behavior - I can reproduce
the unit test with a RoutedCommand as well. If there is no other
reference to the handler it's collected.

The question really is, how common is this scenario? I can't recall
ever need to wire up a handler for CanExecute on a command myself..

mark

[Peter O'Hanlon]

I've only ever hooked up the CanExecute when I want to manage the enabled state of a button/menu item.

--
Peter O'Hanlon

[Bill Kempf]

Programmer: Dr. WPF! Dr. WPF! My CanExecute delegate gets garbage
collected when I do this!
Dr. WPF: Then don't do that!

Sorry, had to be said.

--

[Laurent Bugnion, GalaSoft]

Never did that either. At most, I call the CanExecute method before
executing (or not) a command in code.

I have been wondering how behaviors (like those published by the Expression
team) handle the CanExecute (if at all) though. My guess right now is that
they don't observe it at all.

Need to check that.

Cheers,
Laurent

-----Original Message-----
From: wpf-di...@googlegroups.com [mailto:wpf-di...@googlegroups.com]
On Behalf Of Mark Smith
Sent: Tuesday, September 08, 2009 2:56 PM
To: wpf-di...@googlegroups.com
Subject: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug

[Andrew]

I missed that aspect as well. The caller/listener holding a reference to its own delegate certainly avoids the rooting issue but it's completely non-obvious that that is required. That being said, you definitely don't want to hold a strong reference to the delegate in the event source when the Target is non-null. While there are ways that the event source could try to get around this, I'm not sure it's worth it as it's probably not common.

-Andrew

----- Original Message ----
From: Mark Smith <mark....@gmail.com>
To: wpf-di...@googlegroups.com
Sent: Tuesday, September 8, 2009 7:16:45 AM
Subject: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug

[Eric Burke]

If you change this line:

 

    target.CanExecuteChanged += (s, a) => canExecuteChangedWasCalled =

true;

 

to this:

   

    EventHandler eh = (s, a) => canExecuteChangedWasCalled = true;

    target.CanExecuteChanged += eh;

 

it will work, because it eliminates the unreferenced delegate problem.

 

 

IMO, the original code in question is incorrect because it hooks the RelayCommand.CanExecuteChanged event but never unhooks it.  Since that event is not documented as a WeakEvent (even though it really IS a WeakEvent based on its proxying for CommandManager.RequerySuggested), the caller should treat it as a strong event, even in the context of this TestMethod, and make sure to unhook it.  Once that happens, a reference will be created and the problem will go away.

 

I would kick this bug back as INVALID and require the caller to write:

 

    EventHandler eh = (s, a) => canExecuteChangedWasCalled = true;

    target.CanExecuteChanged += eh;

    ...

    target.CanExecuteChanged -= eh;

 

 

Also, I would definitely make the documentation clear on how this event is working so that anyone listening knows that they need to otherwise hold a reference to the delegate so that it doesn't get collected.

 

---

From: Andrew <webfl...@yahoo.com>
To: wpf-di...@googlegroups.com
Sent: Tuesday, September 8, 2009 9:23:08 AM

[Sacha Barber]

We use CanExecute a lot for disabling button etc
 

---

Date: Tue, 8 Sep 2009 14:11:20 +0100

Subject: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug

From: pete.o...@gmail.com
To: wpf-di...@googlegroups.com

---

View your other email accounts from your Hotmail inbox. Add them now.

[Josh Smith]

This is a great thread.  I am in agreement that the problematic scenario is highly unusual, and in a sense, invalid.  I will close the issue. :)

Thanks,

Josh

[Mike Brown]

Now that I've had my pancakes, bacon, eggs, and coffee, the problem is very clear to me...there isn't one. When the relay command is bound to a command source (e.g. Button or Menu), they properly register for the CanExecuteChanged event (holding a reference to the delegate for the life of the control). The only problem is one of documentation. "If you're implementing your own command source you must hold on to the delegate registered to CanExecuteChanged for the life of your control".

 

Reading the rest of the thread it appears others have pointed this out as well.