CanExcuteChanged

Sep 7, 2009 at 3:53 AM

hello,

Love the framework.  Small but useful.   Unfortunately, I've been having a few problems with the RelayCommand, in particular the CanExecute method not being re-queried.   The RelayCommand directly registers the CanExecuteChanged event handler with the CommandManager.RequestSuggested event.  The MSDN docs say that the CommandManager.RequestSuggested only holds a weak reference to the handler, so shouldn't the RelayCommand also keep a strong reference to the delegate to avoid it being collected?  When CommandManager.InvalidateRequerySuggested is called, the CanExecuteChanged event stops being raised after a GC, however, for some reason buttons seem ok with this and keep re-quering CanExecute.   If I attach my own handler to CanExecuteChanged it is not longer called after a GC.  Any ideas?

Paul

 

Coordinator
Sep 7, 2009 at 4:48 PM

Hey Paul,

Are you keeping a reference to the RelayCommand as a field somewhere?  If not, then I suppose it might be eligible for GC (though I would have suspected that the ICommandSource -- such as a Button -- would root it).

Josh

Sep 7, 2009 at 10:59 PM

Hi Josh,

The RelayCommand is ok, it's the CanExecuteChanged delegate object passed to the RelayCommand  that gets weak referenced, collected and therefore not called. When the Button (or I) attach a CanExecuteChanged handler to the RelayClass, it gets attached to the CommandManager.RequestSuggested event which lets go of the delegate on the next GC - unless I add hold a copy in the RelayCommand it self.  

I thought I'd write you a nice unit test to show this, but its not quite so easy as CommandManager.RequestSuggested is invoked on the dispatcher, so we need a DoEvents methods to ensure it does its thing.  I have borrowed a DoEvents class from Zhou  to make this work.  Here's the unit test and required dispatcher helper.  

    [TestClass]
    public class RelayCommandTests2
    {
        [TestMethod]
        public void CanExecuteChangedDelegateLost()
        {
            //we need a dispatcher running
            Thread t = new Thread( Dispatcher.Run );
            t.Start();

            //do nothing relay command
            RelayCommand target = new RelayCommand( () => { }, () => true );

            //setup a bool that lets us know the event was raised
            bool canExecuteChangedWasCalled = false;
            target.CanExecuteChanged += (s, a) => canExecuteChangedWasCalled = true;

            //simulate a change that requires CanExecute to be queried again
            canExecuteChangedWasCalled = false;
            CommandManager.InvalidateRequerySuggested();
            WpfApplication.DoEvents(); //force the dispatch do its thing
            Assert.IsTrue( canExecuteChangedWasCalled );

            //now collect GC
            GC.Collect();

            //and try again
            canExecuteChangedWasCalled = false;
            CommandManager.InvalidateRequerySuggested();
            WpfApplication.DoEvents();
            Assert.IsTrue( canExecuteChangedWasCalled );
        }
    }


    namespace Sheva.Windows
    {
        /// <summary>
        /// Encapsulates a Windows Presentation Foundation application model with added functionalities.
        /// </summary>
        public class WpfApplication : Application
        {
            private static DispatcherOperationCallback exitFrameCallback = new
                 DispatcherOperationCallback(ExitFrame);

            /// <summary>
            /// Processes all UI messages currently in the message queue.
            /// </summary>
            public static void DoEvents()
            {
                // Create new nested message pump.
                DispatcherFrame nestedFrame = new DispatcherFrame();

                // Dispatch a callback to the current message queue, when getting called, 
                // this callback will end the nested message loop.
                // note that the priority of this callback should be lower than the that of UI event messages.
                DispatcherOperation exitOperation = Dispatcher.CurrentDispatcher.BeginInvoke(
                    DispatcherPriority.Background, exitFrameCallback, nestedFrame);

                // pump the nested message loop, the nested message loop will immediately 
                // process the messages left inside the message queue.
                Dispatcher.PushFrame(nestedFrame);

                // If the "exitFrame" callback doesn't get finished, Abort it.
                if (exitOperation.Status != DispatcherOperationStatus.Completed)
                {
                    exitOperation.Abort();
                }
            }

            private static Object ExitFrame(Object state)
            {
                DispatcherFrame frame = state as DispatcherFrame;

                // Exit the nested message loop.
                frame.Continue = false;
                return null;
            }
        }

 

This test fails after the GC.  To make this test pass, we need to save the CanExecuteChanged delegate in a field in the RelayCommand, as well as passing it on.  Is that a reasonable change?

Paul

Coordinator
Sep 7, 2009 at 11:22 PM

Wow.  I can't believe this issue was never caught until now.  Great catch!!

I'll add the fix asap.

Thanks!
Josh 

Coordinator
Sep 8, 2009 at 3:15 PM

Hi,

I tossed this issue around the WPF Disciples group, and came to the conclusion that this defect should not (cannot?) be fixed.  By fixing the issue you point out, it creates a memory leak due to a defect in WPF. For more information about this decision, please read the Disciples thread.

http://groups.google.com/group/wpf-disciples/browse_thread/thread/4b18a9de327be281

Thanks,

Josh

Sep 8, 2009 at 10:53 PM

Hi,

Ok - sounds reasonable.  Documenting that CanExecuteChanged is held by a weak reference makes it clear.   The WPF controls explicitly hang onto the delegate they pass in so they are unaffected.  If you're writing a ICommandSource, you must explicitly save a reference to the CanExecuteChanged delegate you hook/unhook it.   If you rely on the implicit add/remove handler behaviour in c#, the code will not be leaky, but the delegate will stop getting called after a GC.

 eg.  Add/Remove handler code that will stop getting called after GC

//to hook
doStuff.CanExecuteChanged += doStuff_CanExecuteChanged;

//to unhook
doStuff.CanExecuteChanged -= doStuff_CanExecuteChanged;

.....
private void doStuff_CanExecuteChanged(object sender, EventArgs e)

 

Saving an explicit reference to delegate (doStuff_CanExecuteChanged), although not obvious, will correct this.  

Thanks for the speedy turn around & effort put into this.
Paul

Sep 13, 2009 at 4:30 PM

I've been trying to follow the discussion here and at the Disciples group, but I'm still a bit fuzzy on the solution.  One question - is this the same issue that Nathan Nesbit blogged about here? i.e. that led to his need for the CommandProxy class, or am I mixing up issues?

Tom.

Aug 24, 2012 at 1:16 AM

This took me a long time to find, but thank you so so much!