CompletedAwareCommandTrigger/ICompletionAwareCommand leaking memory

Jul 7, 2010 at 7:28 AM

hi sacha, your CompletedAwareCommandTrigger and there the ICompletionAwareCommand.CommandCompleted is leaking memory. You have to update your SimpleCommand

private event Action<Object> commandCompleted;

public void Execute(T2 parameter)
        {
            if (executeMethod != null)
            {
                executeMethod(parameter);
            }

            //now raise CommandCompleted for this ICommand
            Action<Object> completedHandler = commandCompleted;
            if (completedHandler != null)
            {
                completedHandler(parameter);
            }
        }
 in any way:)

You can test this with your Demoapp (ImageLoaderView.xaml). for now i use a WeakActionEvent<object> instead of event Action<object>, not the best solution but it works :) btw i use your WeakAction for the WeakActionEvent<T> class.

public class WeakActionEvent<T>
    {
        public static WeakActionEvent<T> operator +(WeakActionEvent<T> wre, Action<T> handler)
        {
            wre.Add(handler);
            return wre;
        }

        private void Add(Action<T> handler)
        {
            var parameters = handler.Method.GetParameters();

            if (parameters != null && parameters.Length > 1)
                throw new InvalidOperationException("Action should have only 0 or 1 parameter");

            if (_delegates.Any(del => del.Target == handler.Target))
            {
                return;
            }

            var parameterType = (parameters == null || parameters.Length == 0)
                                ? null
                                : parameters[0].ParameterType;

            _delegates.Add(new WeakAction(handler.Target, handler.Method, parameterType));
        }

        public static WeakActionEvent<T> operator -(WeakActionEvent<T> wre, Action<T> handler)
        {
            wre.Remove(handler);
            return wre;
        }
        private void Remove(Action<T> handler)
        {
            foreach (var del in _delegates)
                if (del.Target == handler.Target)
                {
                    _delegates.Remove(del);
                    return;
                }
        }

        readonly List<WeakAction> _delegates = new List<WeakAction>();

        internal void Invoke(T arg)
        {
            for (var i = _delegates.Count - 1; i > -1; --i)
            {
                var weakAction = _delegates[i];
                if (!weakAction.IsAlive)
                    _delegates.RemoveAt(i);
                else
                {
                    var action = weakAction.CreateAction();
                    action.DynamicInvoke(arg);
                }
            } 
        }
    } 

 /// <summary>
        ///     Execution of the command
        /// </summary>
        public void Execute(T parameter)
        {
            if (_executeMethod != null)
            {
                _executeMethod(parameter);
            }

            this.CommandCompleted.Invoke(parameter);
        }

 
Coordinator
Jul 8, 2010 at 4:01 PM
Edited Jul 8, 2010 at 4:11 PM

I am not sure what you are saying, and I can not see what is wrong. Do you have a proposed fix, I honestly can't see anything wrong after looking at the code. I could do what you suggest, but by my way of thinking I can see nothing wrong with the Trigger or the SimpleCommand class. It all looks correct to me. We hook up the event on Attaching override and unhook on Detaching override

Jul 9, 2010 at 8:58 AM
Edited Jul 9, 2010 at 9:19 AM

hi sacha, to see what i mean just add some memory placeholder to your view in your wpf sample.

public partial class ImageLoaderView : UserControl, IWorkSpaceAware
    {
        public byte[] myMemory = new byte[100 * 1024 * 1024];    //allocate 100MB

...}

if you now start your app and swap between "Image View" and "About Cinch V2" tab you will see memory raising till oom exception because ImageLoaderView gets not collected. the Trigger is fine but the problem is the connection to the completed command.

my fix is changing ICompletionAwareCommand to:

    /// <summary>
    /// Interface that is used for ICommands that notify when they are
    /// completed
    /// </summary>
    public interface ICompletionAwareCommand
    {
        /// <summary>
        /// Notifies that the command has completed
        /// </summary>
        WeakActionEvent<object> CommandCompleted { get; set; }
    }

btw it seems that override OnDetaching is never called. and even if you unhook

this.Command.CommandCompleted -= Command_Completed;

the view is not gc with your current implementation.

 

Coordinator
Jul 9, 2010 at 11:26 AM
Edited Jul 9, 2010 at 11:27 AM

Wierd, how strange, sure I checked that it was getting called. I must of missed that, Ill look into this one this weekend. Thanks man.

 

Can I also just ask how you are finding working with CinchV2?

Jul 9, 2010 at 11:45 AM
Edited Jul 9, 2010 at 11:49 AM

well i'm sorry to say that but i do not use cinch as a framework, cause i just try to understand wpf, mvvm, and mef a little bit better and your codeproject posts helped me a lot  and i really like the mef effect of cinchv2 (thx to marlon^^) unfortunately my projects at work are all without mvvm+mef (we saying: "Wer nur einen Hammer hat, für den sieht jedes Problem wie ein Nagel aus."  and our pm like his ooold hammer^^ and did not like new tools and things...). so i just learn the new things for myself and maybe better times at work^^

ps: my IE8 alert me when going on your site: sachabarber.net. seems to be a trojaner. its since 2-3weeks for now. hopefully its just the sensible virusscanner at work, but maybe you will take a look. our admins did not let me on your site anymore;)

Coordinator
Jul 9, 2010 at 12:43 PM

Fair enough, its good to learn from at least. As far as trojan, goes you are 2nd person to state this, yet my PC at home is always being virus checked and source is really just uploaded to codeplex. Cant see what the feck is causing that. Thanks for your input on the other weak reference thing, I do already have a WeakEvent and WeakProxy did you see those in Cinch codebase under the Events folder

Jul 9, 2010 at 1:19 PM
Edited Jul 9, 2010 at 8:12 PM

sure i look at all your cinch classes :) the WeakAction i use above is simply the following (is a shame i really dont know from which wpf guru i copied this...maybe yours or marlon?)

internal class WeakAction : WeakReference
    {
        #region Data

        readonly MethodInfo _method;
        readonly Type _delegateType;

        #endregion

        #region Internal Methods

        /// <summary>
        /// Constructs a new WeakAction
        /// </summary>
        /// <param name="target">The sender</param>
        /// <param name="method">The _method to call on sender</param>
        /// <param name="parameterType">The parameter type if using generics</param>
        internal WeakAction(object target, MethodInfo method, Type parameterType)
            : base(target)
        {
            this._method = method;
            this._delegateType = parameterType == null
                                 ? typeof(Action)
                                 : typeof(Action<>).MakeGenericType(parameterType);
        }

        /// <summary>
        /// Creates callback delegate
        /// </summary>
        /// <returns>Callback delegate</returns>
        internal Delegate CreateAction()
        {
            var target = base.Target;
            if (target != null)
            {
                // Rehydrate into a real Action
                // object, so that the _method
                // can be invoked on the target.
                return Delegate.CreateDelegate(this._delegateType, base.Target, this._method);
            }

            return null;
        }
        #endregion
    }

btw if you ever make behaviors  with something like this

//Causes MemoryLeak
DependencyPropertyDescriptor focusProp = DependencyPropertyDescriptor.FromProperty(UIElement.IsFocusedProperty, typeof(FrameworkElement));
if (focusProp != null)
{
     focusProp.AddValueChanged(AssociatedObject, (s, args) => UpdateAdorner());
}

you hit the next memory leak :) the solution comes here.

 

Coordinator
Jul 9, 2010 at 2:12 PM

Yeah that code was Marlons. Andrew Smith is "THE MAN" even Josh calls him a guru. Luckily I never do attached DPs like that.

What does piss me off, if what you say is true is that Microsofts own code templates promote the user into thinking that a detach override will occur (and it should), why events cant just be weak from the start.

 

Also what I was saying was that Cinch V1 had weakevent support out of the box, using WeakEvent and WeakProxy, so I believe you could have used those instead. Same results though.

 

Thanks really for pointing that out.

Coordinator
Jul 9, 2010 at 4:28 PM

Actually I think this could actually be a TabControl issue and an issue that for the demo apps the VMs are shared, and not been given a new instance each time. But I will look into this one.

Jul 9, 2010 at 5:10 PM

i have a demo app with datatemplates viewmodel first approach and nonshared vm's. its the same issue. 

the problem is the VIEW gets not garbage collected :) so have fun looking into this and a nice weekend.

Coordinator
Jul 9, 2010 at 7:57 PM
WeakEvent it is then, cheers. So one thing, do you think I have done a good job with Cinch/CinchV2?
Jul 9, 2010 at 8:21 PM

what a question :) of course you did a great work with cinch+V2. i really learned alot about wpf and mvvm over the last year from cinch and the other mvvm frameworks out there. inet rulez^^ atm i really like the mef+mvvm idea from marlon.

Coordinator
Jul 9, 2010 at 8:26 PM
Edited Jul 9, 2010 at 8:29 PM
Me too that's why Marlon and I agreed to do things like this, I too had my own MEF thing going on using something Glenn Brook (MEF PM) was working on, but I talked with Marlon and he helped me get it to work nice with Cinch. I love Marlon hes my man. I should have that weak event thing fixed this weekend. I think I need to show people how the TabControl is wrong when using attached props to find VMs. The TabControl in WPF is pure evil, as it creates new selected tab (View) from stratch each time, so that means MEF would try and create a new one for you, eeeks. Lost state. Arggg So you have to use a specialised TabControl which I wrote about in CinchV1 that keeps all items in memory unless they asked to close. Grrrr, MSFT need to fix that
Jul 9, 2010 at 9:22 PM

in my testapps use listbox + contentcontrol as replacement for tabcontrol. works like a charm with viewmodel first and datatemplates. i'm off for today. 

Coordinator
Jul 10, 2010 at 6:48 AM
If you have time, I'd love to see a small demo app of your listbox + contentcontrol replacement. Can you send it to sacha[DOT]barber[AT]gmail[DOT]com
Coordinator
Jul 10, 2010 at 8:05 AM
Edited Jul 10, 2010 at 8:28 AM
Could you please post your full SimpleCommand<T1,T2> code up here, as I can not get your code to work, as I can not get my implementation to work. I would love to see how you solved it. For example what does your actual get/set property look like, I do not get what you would be doing in there. Please could you post the Full code up here. I really need a hand with this one man. Please could you help me out and send me your code for SimpleCommand.
Jul 10, 2010 at 12:05 PM
Edited Jul 10, 2010 at 12:07 PM

hi sacha, i just have the code at work :) so you have to wait till monday. i actually dont use your simplecommand, but the Delegatecommand from the mvvm template. i just add your ICompletioAwareCommand interface to it.

 

 /// <summary>
    /// Interface that is used for ICommands that notify when they are
    /// completed
    /// </summary>
    public interface ICompletionAwareCommand
    {
        /// <summary>
        /// Notifies that the command has completed
        /// </summary>
        WeakActionEvent<object> CommandCompleted { get; set; }
    }

 

but of course i will post the whole code here on monday. as far as i remember i just put this property to the Delegatecommand and call invoke on exectue:

 

/// <summary>
        ///     Execution of the command
        /// </summary>
        public void Execute(T parameter)
        {
            if (_executeMethod != null)
            {
                _executeMethod(parameter);
            }

            this.CommandCompleted.Invoke(parameter);
        }

i'm out for relaxing on the beach now, btw 38°C ...  :)

 

 

Coordinator
Jul 11, 2010 at 7:42 AM
Yeah I cant see where you set the CommandCompleted SET in the 1st place, I was getting null references. Would love to see your code on Monday. Also I am not sure this is the problem, as if I comment out these 2 triggers in that Views XAML, and add a deconstructor to that view and make that views Tab closeable, the deconstructor is still never called. Need to investigate further. But could you still post your code on Monday
Jul 12, 2010 at 5:45 AM
Edited Jul 12, 2010 at 6:15 AM

i create the CommandCompleted in the contructor :) maybe not a perfect soltution but it works^^

 public class DelegateCommand<T> : ICommand, ICompletionAwareCommand
    {
        #region Data

        private readonly Action<T> _executeMethod = null;
        private readonly Func<T, bool> _canExecuteMethod = null;
        private bool _isAutomaticRequeryDisabled = false;
        private List<WeakReference> _canExecuteChangedHandlers;

        #endregion

        #region Constructors

        /// <summary>
        ///     Constructor
        /// </summary>
        public DelegateCommand(Action<T> executeMethod)
            : this(executeMethod, null, false)
        {
        }

        /// <summary>
        ///     Constructor
        /// </summary>
        public DelegateCommand(Action<T> executeMethod, Func<T, bool> canExecuteMethod)
            : this(executeMethod, canExecuteMethod, false)
        {
        }

        /// <summary>
        ///     Constructor
        /// </summary>
        public DelegateCommand(Action<T> executeMethod, Func<T, bool> canExecuteMethod, bool isAutomaticRequeryDisabled)
        {
            if (executeMethod == null)
            {
                throw new ArgumentNullException("executeMethod");
            }

            _executeMethod = executeMethod;
            _canExecuteMethod = canExecuteMethod;
            _isAutomaticRequeryDisabled = isAutomaticRequeryDisabled;
            this.CommandCompleted = new WeakActionEvent<object>();
        }

        #endregion

        #region Public Methods

        /// <summary>
        ///     Method to determine if the command can be executed
        /// </summary>
        public bool CanExecute(T parameter)
        {
            if (_canExecuteMethod != null)
            {
                return _canExecuteMethod(parameter);
            }
            return true;
        }

        /// <summary>
        ///     Execution of the command
        /// </summary>
        public void Execute(T parameter)
        {
            if (_executeMethod != null)
            {
                _executeMethod(parameter);
            }

            this.CommandCompleted.Invoke(parameter);
        }

        /// <summary>
        ///     Raises the CanExecuteChaged event
        /// </summary>
        public void RaiseCanExecuteChanged()
        {
            OnCanExecuteChanged();
        }

        /// <summary>
        ///     Protected virtual method to raise CanExecuteChanged event
        /// </summary>
        protected virtual void OnCanExecuteChanged()
        {
            CommandManagerHelper.CallWeakReferenceHandlers(_canExecuteChangedHandlers);
        }

        /// <summary>
        ///     Property to enable or disable CommandManager's automatic requery on this command
        /// </summary>
        public bool IsAutomaticRequeryDisabled
        {
            get
            {
                return _isAutomaticRequeryDisabled;
            }
            set
            {
                if (_isAutomaticRequeryDisabled != value)
                {
                    if (value)
                    {
                        CommandManagerHelper.RemoveHandlersFromRequerySuggested(_canExecuteChangedHandlers);
                    }
                    else
                    {
                        CommandManagerHelper.AddHandlersToRequerySuggested(_canExecuteChangedHandlers);
                    }
                    _isAutomaticRequeryDisabled = value;
                }
            }
        }

        #endregion

        #region ICommand Members

        /// <summary>
        ///     ICommand.CanExecuteChanged implementation
        /// </summary>
        public event EventHandler CanExecuteChanged
        {
            add
            {
                if (!_isAutomaticRequeryDisabled)
                {
                    CommandManager.RequerySuggested += value;
                }
                CommandManagerHelper.AddWeakReferenceHandler(ref _canExecuteChangedHandlers, value, 2);
            }
            remove
            {
                if (!_isAutomaticRequeryDisabled)
                {
                    CommandManager.RequerySuggested -= value;
                }
                CommandManagerHelper.RemoveWeakReferenceHandler(_canExecuteChangedHandlers, value);
            }
        }

        bool ICommand.CanExecute(object parameter)
        {
            // if T is of value type and the parameter is not
            // set yet, then return false if CanExecute delegate
            // exists, else return true
            if (parameter == null &&
                typeof(T).IsValueType)
            {
                return (_canExecuteMethod == null);
            }
            return CanExecute((T)parameter);
        }

        void ICommand.Execute(object parameter)
        {
            Execute((T)parameter);
        }

        #endregion

        #region Implementation of ICompletionAwareCommand

        public WeakActionEvent<object> CommandCompleted { get; set;}

        #endregion
    }
i will send you my prototype app too, maybe you can tell me some improvements :)
Coordinator
Jul 23, 2010 at 10:38 AM

All fixed in new codebase...Thanks for your help