Command InvalidateCanExecute

[Henri]

I want to report that the 2011 Q1 DelegateCommand does not work the same as 2010 Q3.
In q3, I could use non-radbuttons. It no longer works in 2011 q1.
Best regards,
Henri

[Hristo]

Hello Henri,

We changed the DelegateCommand to use weak event pattern. This change was done in order to prevent memory leak on RadControls (e.g. RadControl that is bound to DelegateCommand will be alive because it have strong reference to the DelegateCommand).

This change cause ButtonBase to loose its handler to CanExecuteChanged event.
Please use RadButtons or you could create your own implementation of ICommand class that is not using weak event pattern (but keep in mind that it will probably cause memory leaks).

Greetings,
Hristo
the Telerik team

[Henri]

Thank you for the explanation.
Best regards,
Henri

[Kirill]

Thanks for the explanation, but would you mind explaining a little further?

.net is designed to not cause memory leaks under circumstances of circular referencing, which is, as far as I can understand what you're saying. 

Do I understand you correctly? You seem to be saying that the following would cause a memory leak, but I tested this scenario and it seems to be fine.

executing the following code...

for (long i = 0; i < reps; i++)
{
    C c = new C();
    B b = new B(c);
}

for the following classes ...

public class B

{

    C C = null;

    public B(C c)

    {

        C = c;

        C.Event += new EventHandler(C_Event);

    }

 

    void C_Event(object sender, EventArgs e)

    {

        //dosomething

    }

}

 

public class C

{

    public event EventHandler Event;

}


[Hristo]

Hello Kirill,

In the case of DelegateCommand - most of the time it is used in a ViewModel.
When RadButton is bound to such command it will add handler to CanExecuteChanged event. Then if you switch to another view but use the same view model then the old view will still be alive (because of the radbutton holding a strong reference to DelegateCommand which is also alive).

In you sample all instances are disposed after the "for" scope so there will be no memory leak.

Best wishes,
Hristo
the Telerik team

[Jonx]

Hello Hristo,
So, if I get you correctly, DelegateCommand is only working correctly with radcontrols?

The thing is that I lost my weekend at trying to find why InvalidateCanExecute did only trigger the CanExecute event once... because like you say "ButtonBase is loosing its handler to CanExecuteChanged event. That's what I saw... This was code working in the previous version like stated by Kirill, so I trusted your code more then mine and tried to figure out what I had changed in my code that made my binding to those command stop working.

At the end I wrote my own command which did work.

Now, would be me so kind and point me to the release note of your controls that document the breaking change?
I'm always reading them carrefully but I must have missed the part where it says that previous working code will stop working...

Also, would you be even more kind and add to the documentation of DelegateCommand that it should not be used with non RadControls. That may help poor programmers like me stop wasting time debuging their code when it's your code that changed...

Thanks a lot in advance,
John.

[Hristo]

Hi John,

I'm sorry that you had to lost your weekend for this issue.
It appears that we have missed to include this change in the release notes. Thank you for point it. We will update our release notes to include this change. I've updated your telerik points.

As for the change I'll try to explain it in details:

Before 2011 Q1 our DelegateCommand was using normal CanExecuteChanged event. RadControls that support ICommand are using instance field to keep a reference to this event when Command property is set.
In this scenario if you have a ViewModel that expose ICommand (and internally use our DelegateCommand) then you will observe memory leak - e.g. when the view is changed it will not be released from memory because the DelegateCommand will keep a reference to it so that it can call the delegate.

We had to change DelegateCommand in order to prevent this memory leak. Now the problem that arise from this change is that controls that do not keep reference to CanExecuteChanged event handler (like ButtonBase) will lose this reference when garbage collector starts.
There was another solution - to keep DelegateCommand and change all controls to keep weak references to CanExecuteChanged handler. But this would cause all other (non RadControls like ButtonBase) to do memory leak. (ButtonBase does not provide virtual method when Command property changed so we could not fix it).

In the end there is no perfect solution - if controls keep strong reference to CanExecuteChanged handler then the command should use weak event pattern internally to prevent memory leak. If controls do not keep reference (or keep weak reference) to CanExecuteChanged handler Command should NOT use weak event pattern or event handlers will be garbage collected.

I hope that this justify our decision.

All the best,
Hristo
the Telerik team

[Jonx]

Hello Hristo,

Of course this does justify your decisions, the only thing I had concerns with was the fact that I was left in the dark about the impact of such a change...

I'm sure everybody here immediately understood what was at stake and had no trouble correcting their problems but I'm not that good and if I don't have someone taking my hand and tell me, watch out John, here we made a change that will make your command stop working if you don't replace your controls by RadControls, then I lose lot's of time trying to figure out what the hell is going on with my code...

Anyway, thank you for the detailed explanation...

John.

[Paul Dhertoghe]

Just ran into this issue also, glad it is covered in this Forum so I could quickly understand what is going on and fix it.

Telerik should definitely make more work to communicate breaking changes to developers via release notes; since we started working with Telerik controls in april 2010, second time we hit an undocumented breaking change.

With kind regards,

Paul D'hertoghe