2
Vote

DelegateCommand should use DelegateReference to reference target object

description

It's a common complaint that the CanExecuteChanged handlers of DelegateCommand get unexpectedly garbage collected. While it's simple enough to work around the problem, I think it would be worth changing Prism to avoid the problem appearing:
 
In DelegateCommand, rather than referencing the EventHandler with a WeakReference for the CanExecuteChanged handlers, Prism should instead use the DelegateReference (as used by the Event Aggregator).
 
DelegateReference holds a weak referece to the target object of the Delegate, instead of the handler function itself.
 
This would then negate the need for holding an explicit reference to the EventHandler for the majority of handlers, and would still not present a memory leak.

comments

aadami wrote Jan 27, 2012 at 5:31 PM

Hi,

Based on my understanding this is currently by design as it can be seen in the following comments inside the Prism v4 Library source code (DelegateCommandBase.cs):
    /// 
    /// Occurs when changes occur that affect whether or not the command should execute. You must keep a hard
    /// reference to the handler to avoid garbage collection and unexpected results. See remarks for more information.
    /// 
    /// 
    /// When subscribing to the  event using 
    /// code (not when binding using XAML) will need to keep a hard reference to the event handler. This is to prevent 
    /// garbage collection of the event handler because the command implements the Weak Event pattern so it does not have
    /// a hard reference to this handler. An example implementation can be seen in the CompositeCommand and CommandBehaviorBase
    /// classes. In most scenarios, there is no reason to sign up to the CanExecuteChanged event directly, but if you do, you
    /// are responsible for maintaining the reference.
    /// 
    /// 
    /// The following code holds a reference to the event handler. The myEventHandlerReference value should be stored
    /// in an instance member to avoid it from being garbage collected.
    /// 
    /// EventHandler myEventHandlerReference = new EventHandler(this.OnCanExecuteChanged);
    /// command.CanExecuteChanged += myEventHandlerReference;
    /// 
    /// 
    public event EventHandler CanExecuteChanged
    {
        add
        {
            WeakEventHandlerManager.AddWeakReferenceHandler(ref _canExecuteChangedHandlers, value, 2);
        }
        remove
        {
            WeakEventHandlerManager.RemoveWeakReferenceHandler(_canExecuteChangedHandlers, value);
        }
    }
We are leaving this work item open, so that the team is aware of your suggestion and analyzes it for future releases.

Thanks,

Agustin Adami
http://blogs.southworks.net/aadami

barakbbn wrote Jul 8, 2015 at 1:44 PM

We recently encountered a nasty bug that found to be since DelegateCommand internal implementation hold weak reference of the delegate.
Alghouth you claim it's by design, you need to realize how is the common usage, intuative and acceptable event registration C# by most developers, and realize they will do simple CanExecuteChanged += MyHandlingMethod and will get hit by curent implementation when it's GCed.

In addition when commonly implementing ICommandSource it isn't trivial to check or assume Prism DelegateCommand is under our ICommand and therefor keep the delegate hard referenced locally.
Or if I have i other class an ICommand to always keep hard reference to delegate, since who knows if it's Prism DelegateCommand. it's not the common practice.
keep it simple: command.CanexecuteChanged += MyHanlingMethod

Although it's easy to wave the flag of it's by design, it actually seems like design bug or wrong design,
because you trick most developers that custom to work a very common way.
you can't expect developers to read the class comments that it's required to "You must keep a hard reference to the handler ", they have know good reason to start digging in your library in order to do simple things as registering to events.

Today you have other better implementation or handling weak delegates that won't fool developers, and you ought to take it seriously to change current implementation to keep it simple.

thanks