Account Options

  1. Sign in
The old Google Groups will be going away soon.
Switch to the new Google Groups.
Google Groups Home
« Groups Home
XAML Disciples
Home
About this group
Contact owner to join
RelayCommand: memory leak vs. behavior bug
Options
There are currently too many topics in this group that display first. To make this topic appear first, remove this option from another topic.
There was an error processing your request. Please try again.
flag
   Messages 1 - 25 of 27 - Collapse all  -  Translate all to Translated (View all originals)   Newer >
The group you are posting to is a Usenet group. Messages posted to this group will make your email address visible to anyone on the Internet.
Your reply message has not been sent.
Your post was successful
 
From:
To:
Cc:
Followup To:
Add Cc | Add Followup-to | Edit Subject
Subject:
Validation:
For verification purposes please type the characters you see in the picture below or the numbers you hear by clicking the accessibility icon. Listen and type the numbers you hear
 
Josh Smith  
View profile  
 More options Sep 7 2009, 7:39 pm
From: Josh Smith <flappleja...@gmail.com>
Date: Mon, 7 Sep 2009 16:39:07 -0700
Local: Mon, Sep 7 2009 7:39 pm
Subject: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

Hey all,
Someone just pointed out a bug in RelayCommand.

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

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Karl Shifflett  
View profile  
 More options Sep 7 2009, 8:24 pm
From: "Karl Shifflett" <molena...@comcast.net>
Date: Mon, 7 Sep 2009 17:24:59 -0700
Local: Mon, Sep 7 2009 8:24 pm
Subject: RE: [WPF Disciples] RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

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

From: wpf-disciples@googlegroups.com [mailto:wpf-disciples@googlegroups.com]
On Behalf Of Josh Smith
Sent: Monday, September 07, 2009 4:39 PM
To: wpf-disciples@googlegroups.com
Subject: [WPF Disciples] RelayCommand: memory leak vs. behavior bug

Hey all,

Someone just pointed out a bug in RelayCommand.  

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

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Karl Shifflett  
View profile  
 More options Sep 7 2009, 8:46 pm
From: "Karl Shifflett" <molena...@comcast.net>
Date: Mon, 7 Sep 2009 17:46:25 -0700
Local: Mon, Sep 7 2009 8:46 pm
Subject: RE: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

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

From: wpf-disciples@googlegroups.com [mailto:wpf-disciples@googlegroups.com]
On Behalf Of Karl Shifflett
Sent: Monday, September 07, 2009 5:25 PM
To: wpf-disciples@googlegroups.com
Subject: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug

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

From: wpf-disciples@googlegroups.com [mailto:wpf-disciples@googlegroups.com]
On Behalf Of Josh Smith
Sent: Monday, September 07, 2009 4:39 PM
To: wpf-disciples@googlegroups.com
Subject: [WPF Disciples] RelayCommand: memory leak vs. behavior bug

Hey all,

Someone just pointed out a bug in RelayCommand.  

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

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

  RelayCommandNoGC.zip.doc
35K Download

 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Bill Kempf  
View profile  
 More options Sep 7 2009, 9:12 pm
From: Bill Kempf <weke...@gmail.com>
Date: Mon, 7 Sep 2009 21:12:03 -0400
Local: Mon, Sep 7 2009 9:12 pm
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author
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.


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Karl Shifflett  
View profile  
 More options Sep 7 2009, 9:21 pm
From: "Karl Shifflett" <molena...@comcast.net>
Date: Mon, 7 Sep 2009 18:21:40 -0700
Local: Mon, Sep 7 2009 9:21 pm
Subject: RE: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author
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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Karl Shifflett  
View profile  
 More options Sep 7 2009, 9:25 pm
From: "Karl Shifflett" <molena...@comcast.net>
Date: Mon, 7 Sep 2009 18:25:15 -0700
Local: Mon, Sep 7 2009 9:25 pm
Subject: RE: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author
Bill & Josh,

Still works with Lambda's.

Karl


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Bill Kempf  
View profile  
 More options Sep 7 2009, 9:35 pm
From: Bill Kempf <weke...@gmail.com>
Date: Mon, 7 Sep 2009 21:35:58 -0400
Local: Mon, Sep 7 2009 9:35 pm
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author
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

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

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Karl Shifflett  
View profile  
 More options Sep 7 2009, 9:42 pm
From: "Karl Shifflett" <molena...@comcast.net>
Date: Mon, 7 Sep 2009 18:42:45 -0700
Local: Mon, Sep 7 2009 9:42 pm
Subject: RE: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author
Yes, my lambda does call a member.

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Andrew  
View profile  
 More options Sep 7 2009, 11:08 pm
From: Andrew <webflye...@yahoo.com>
Date: Mon, 7 Sep 2009 20:08:18 -0700 (PDT)
Local: Mon, Sep 7 2009 11:08 pm
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author
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

On Sep 7, 2009, at 9:42 PM, "Karl Shifflett" <molena...@comcast.net> wrote:

Yes, my lambda does call a member.

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Mark Smith  
View profile  
 More options Sep 8 2009, 12:22 am
From: Mark Smith <mark.jul...@gmail.com>
Date: Mon, 7 Sep 2009 23:22:51 -0500
Local: Tues, Sep 8 2009 12:22 am
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

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

On Sep 7, 2009, at 10:08 PM, Andrew wrote:


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Mark Smith  
View profile  
 More options Sep 8 2009, 12:27 am
From: Mark Smith <mark.jul...@gmail.com>
Date: Mon, 7 Sep 2009 23:27:18 -0500
Local: Tues, Sep 8 2009 12:27 am
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

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

On Sep 7, 2009, at 7:24 PM, Karl Shifflett wrote:


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Eric Burke  
View profile  
 More options Sep 8 2009, 12:38 am
From: Eric Burke <ebu...@yahoo.com>
Date: Mon, 7 Sep 2009 21:38:05 -0700 (PDT)
Local: Tues, Sep 8 2009 12:38 am
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

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 <webflye...@yahoo.com>
To: "wpf-disciples@googlegroups.com" <wpf-disciples@googlegroups.com>
Sent: Monday, September 7, 2009 11:08:18 PM
Subject: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug

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

On Sep 7, 2009, at 9:42 PM, "Karl Shifflett" <molena...@comcast.net> wrote:

Yes, my lambda does call a member.

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Josh Smith  
View profile  
 More options Sep 8 2009, 1:09 am
From: Josh Smith <flappleja...@gmail.com>
Date: Mon, 7 Sep 2009 22:09:52 -0700
Local: Tues, Sep 8 2009 1:09 am
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Philipp Sumi  
View profile  
 More options Sep 8 2009, 3:10 am
From: "Philipp Sumi" <phil...@hardcodet.net>
Date: Tue, 8 Sep 2009 09:10:46 +0200
Local: Tues, Sep 8 2009 3:10 am
Subject: RE: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

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

From: wpf-disciples@googlegroups.com [mailto:wpf-disciples@googlegroups.com] On Behalf Of Josh Smith
Sent: Dienstag, 8. September 2009 07:10
To: wpf-disciples@googlegroups.com
Subject: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug

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

On Mon, Sep 7, 2009 at 9:38 PM, Eric Burke <ebu...@yahoo.com> wrote:

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 <webflye...@yahoo.com>
To: "wpf-disciples@googlegroups.com" <wpf-disciples@googlegroups.com>
Sent: Monday, September 7, 2009 11:08:18 PM

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

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

On Sep 7, 2009, at 9:42 PM, "Karl Shifflett" <molena...@comcast.net> wrote:

Yes, my lambda does call a member.

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Mike Brown  
View profile  
 More options Sep 8 2009, 3:13 am
From: Mike Brown <mbrow...@gmail.com>
Date: Tue, 8 Sep 2009 03:13:28 -0400
Local: Tues, Sep 8 2009 3:13 am
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

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.


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Jaime Rodriguez  
View profile  
 More options Sep 8 2009, 3:47 am
From: Jaime Rodriguez <jai...@microsoft.com>
Date: Tue, 8 Sep 2009 07:47:43 +0000
Local: Tues, Sep 8 2009 3:47 am
Subject: RE: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

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)..

From: wpf-disciples@googlegroups.com [mailto:wpf-disciples@googlegroups.com] On Behalf Of Josh Smith
Sent: Monday, September 07, 2009 10:10 PM
To: wpf-disciples@googlegroups.com
Subject: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug

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

On Mon, Sep 7, 2009 at 9:38 PM, Eric Burke <ebu...@yahoo.com<mailto:ebu...@yahoo.com>> wrote:

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 <webflye...@yahoo.com<mailto:webflye...@yahoo.com>>
To: "wpf-disciples@googlegroups.com<mailto:wpf-disciples@googlegroups.com>" <wpf-disciples@googlegroups.com<mailto:wpf-disciples@googlegroups.com>>
Sent: Monday, September 7, 2009 11:08:18 PM

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

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

On Sep 7, 2009, at 9:42 PM, "Karl Shifflett" <molena...@comcast.net<mailto:molena...@comcast.net>> wrote:

Yes, my lambda does call a member.

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Peter O'Hanlon  
View profile  
 More options Sep 8 2009, 3:50 am
From: "Peter O'Hanlon" <pete.ohan...@gmail.com>
Date: Tue, 8 Sep 2009 08:50:03 +0100
Local: Tues, Sep 8 2009 3:50 am
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

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.

On Tue, Sep 8, 2009 at 8:47 AM, Jaime Rodriguez <jai...@microsoft.com>wrote:

--
Peter O'Hanlon

 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Mark Smith  
View profile  
 More options Sep 8 2009, 7:16 am
From: Mark Smith <mark.jul...@gmail.com>
Date: Tue, 8 Sep 2009 06:16:45 -0500
Local: Tues, Sep 8 2009 7:16 am
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author
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

On Sep 8, 2009, at 2:13 AM, Mike Brown wrote:


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Mark Smith  
View profile  
 More options Sep 8 2009, 8:55 am
From: Mark Smith <mark.jul...@gmail.com>
Date: Tue, 8 Sep 2009 07:55:57 -0500
Local: Tues, Sep 8 2009 8:55 am
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

> 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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Peter O'Hanlon  
View profile  
 More options Sep 8 2009, 9:11 am
From: "Peter O'Hanlon" <pete.ohan...@gmail.com>
Date: Tue, 8 Sep 2009 14:11:20 +0100
Local: Tues, Sep 8 2009 9:11 am
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

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

--
Peter O'Hanlon

 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Bill Kempf  
View profile  
 More options Sep 8 2009, 9:11 am
From: Bill Kempf <weke...@gmail.com>
Date: Tue, 8 Sep 2009 09:11:58 -0400
Local: Tues, Sep 8 2009 9:11 am
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author
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.

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

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Laurent Bugnion, GalaSoft  
View profile  
 More options Sep 8 2009, 9:16 am
From: "Laurent Bugnion, GalaSoft" <laur...@galasoft.ch>
Date: Tue, 8 Sep 2009 15:16:07 +0200
Local: Tues, Sep 8 2009 9:16 am
Subject: RE: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author
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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Andrew  
View profile  
 More options Sep 8 2009, 9:23 am
From: Andrew <webflye...@yahoo.com>
Date: Tue, 8 Sep 2009 06:23:08 -0700 (PDT)
Local: Tues, Sep 8 2009 9:23 am
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author
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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Eric Burke  
View profile  
 More options Sep 8 2009, 9:30 am
From: Eric Burke <ebu...@yahoo.com>
Date: Tue, 8 Sep 2009 06:30:32 -0700 (PDT)
Local: Tues, Sep 8 2009 9:30 am
Subject: Re: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

If you change this line:

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

to this:
        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:
 
    EventHandlereh = (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 <webflye...@yahoo.com>
To: wpf-disciples@googlegroups.com
Sent: Tuesday, September 8, 2009 9:23:08 AM
Subject: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug

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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Sacha Barber  
View profile  
 More options Sep 8 2009, 9:41 am
From: Sacha Barber <sachabar...@hotmail.com>
Date: Tue, 8 Sep 2009 13:41:22 +0000
Local: Tues, Sep 8 2009 9:41 am
Subject: RE: [WPF Disciples] Re: RelayCommand: memory leak vs. behavior bug
Print | Individual message | Show original | Report this message | Find messages by this author

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.ohan...@gmail.com
To: wpf-disciples@googlegroups.com

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

On Tue, Sep 8, 2009 at 1:55 PM, Mark Smith <mark.jul...@gmail.com> wrote:

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

_________________________________________________________________
View your other email accounts from your Hotmail inbox. Add them now.
http://clk.atdmt.com/UKM/go/167688463/direct/01/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Messages 1 - 25 of 27   Newer >
« Back to Discussions « Newer topic     Older topic »

Create a group - Google Groups - Google Home - Terms of Service - Privacy Policy
©2012 Google