New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CanExecute WeakRef is cleared when CanExecute-Lambda is in DisplayClass #1192

Open
bluejack2000 opened this Issue May 29, 2018 · 9 comments

Comments

Projects
None yet
2 participants
@bluejack2000

bluejack2000 commented May 29, 2018

This one gave me quite some headaches. CanExecute does not work properly depending on where the lambda for it is compiled into. In version 4.5.3 there was no problem. Problems came after upgrade to 5.5.0

Steps to reproduce

  1. I made a unit test for it

Platform: Windows
.NET version: 4.5.2

Expected behaviour

CanExecute must execute in all cases. Independent whether the compiler puts it in a DisplayClass for whatever reason or not.

Actual behaviour

CanExecute lambda callback gets lost after garbage collection when the lambda is compiled into a DisplayClass.

I forced the compiler to use a DisplayClass for the CanExecute lambda by refering a local variable. I checked with ILSpy that this is the case. Once the lambda is put inside a DisplayClass the weak reference will assume that the Target (which is the DisplayClass) is not alive anymore after next GC causing CanExecute to return true in all cases.

Here is my unit test which fails:

  public class TestDisplayClassViewModel : ViewModelBase
  {
    public TestDisplayClassViewModel()
    {
      int localVariable = 1;
      TestCommand = new Command(TestFunction, () =>
      {
        Debug.WriteLine("CanExecute called " + localVariable++);
        return false;
      });
    }

    public void TestFunction()
    {
    }

    public Command TestCommand { get; set; }
  }

  public class CatelCanExecuteWeakRefTest
  {
    [Test]
    public void CanExecuteWeakRefLostTest()
    {
      Catel.Logging.LogManager.AddDebugListener();
      var vm = new TestDisplayClassViewModel();

      var canExecuteBefore = vm.TestCommand.CanExecute();
      Debug.WriteLine("CanExecute before: " + canExecuteBefore);
      GC.Collect();
      var canExecuteAfter = vm.TestCommand.CanExecute();
      Debug.WriteLine("CanExecute after: " + canExecuteAfter);
      Assert.That(canExecuteAfter, Is.False);
    }
  }

Resulting log is:

CanExecute called 1
CanExecute before: False
19:58:48:783 => [DEBUG] [Catel.WeakFunc`1] [11] Target for 'Boolean <.ctor>b__0()' is no longer alive, weak event being garbage collected
CanExecute after: True

@bluejack2000

This comment has been minimized.

bluejack2000 commented May 29, 2018

This may be related to this discussion

@GeertvanHorrik

This comment has been minimized.

Member

GeertvanHorrik commented May 29, 2018

Does this happen both in release and debug modes?

@bluejack2000

This comment has been minimized.

bluejack2000 commented May 30, 2018

Yes this happens in release mode too. Have to change the Debug.WriteLine to Console.WriteLine ofcourse.

The thing is I don't really understand yet how the .NET compiler decides to put the lambda expression into a DisplayClass. I have code where I just check () => !HasErrors and it gets into a DisplayClass from time to time even though HasErrors is a member. As soon as that happens CanExecute gives true after the next garbage collection.

I wonder why CTL-1007 was implemented in the first place. The lambda should be alive as long as the command is referenced. So it must be referenced by the command. Not by a weak reference.

@bluejack2000

This comment has been minimized.

bluejack2000 commented May 30, 2018

As a workaround I use a derived class CatelCommand for now which keeps a reference to the CanExecute func. That passes the unit test above.

  public class CatelCommand : Command
  {
    private Func<bool> _canExecuteRef;

    public CatelCommand(Action execute, Func<bool> canExecute = null, object tag = null) : base(execute, canExecute, tag)
    {
      _canExecuteRef = canExecute;
    }
  }

  public class CatelCommand<TExecuteParameter, TCanExecuteParameter> : Command<TExecuteParameter, TCanExecuteParameter>
  {
    private object _canExecuteRef;

    public CatelCommand(Action execute, Func<bool> canExecute = null, object tag = null) : base(execute, canExecute, tag)
    {
      _canExecuteRef = canExecute;
    }

    public CatelCommand(Action<TExecuteParameter> execute, Func<TCanExecuteParameter, bool> canExecute = null, object tag = null) : base(execute, canExecute, tag)
    {
      _canExecuteRef = canExecute;
    }
  }

  public class CatelCommand<TExecuteParameter> : Command<TExecuteParameter>
  {
    private object _canExecuteRef;

    public CatelCommand(Action execute, Func<bool> canExecute = null, object tag = null) : base(execute, canExecute, tag)
    {
      _canExecuteRef = canExecute;
    }

    public CatelCommand(Action<TExecuteParameter> execute, Func<TExecuteParameter, bool> canExecute = null, object tag = null) : base(execute, canExecute, tag)
    {
      _canExecuteRef = canExecute;
    }
  }
@bluejack2000

This comment has been minimized.

bluejack2000 commented May 30, 2018

CTL-1007 in my opinion was a misunderstanding and at least the changes to Command should be reverted. There can't be CanExecute "listeners", only the CanExecuteChanged event can have listeners.

CanExecuteChanged event should be implemented as weak event pattern as proposed in CTL-1024 not the CanExecute callback.

@GeertvanHorrik

This comment has been minimized.

Member

GeertvanHorrik commented May 30, 2018

I think reverting is a big step. I am trying to finish a few deadlines. Maybe we created it to prevent a canexecute in combination with CompositeCommand since that contains a list of commands, but we could argue that it's up to the caller to unsubscribe there.

But if you don't, all items you subscribe to will be kept in memory because they are referenced by the global command (e.g. view models kept in memory, etc).

But then again, if you correctly unsubscribe you won't have that issue.

@bluejack2000

This comment has been minimized.

bluejack2000 commented May 30, 2018

As it is at the moment Catel commands can't be used. I had all types of issues with CanExecute suddenly returning true in several parts of our application.

To fix the issue all what has to be done is to remove the weak reference to the CanExecute func in Command, making it a simple reference. Lambdas (which should be a quite common usage scenario for CanExecute) are in danger to get cleaned up by accident with the current implementation.

If you don't have the time I can prepare a pull request. The command must reference the lambda or it can get lost.

As reference, in my link about the compiler discussion he even mentioned the potential problem that arised here:

If we use the lambda expression with weak reference object like the command or messenger for MVVM, it will be collected by GC.

I can understand the notClosure expression will be collected, but I think the closure expression should not be collected due to other code.

If the closure expression will be collected by GC, I am afraid to use the lambda expression because it will cause some defects.

@GeertvanHorrik

This comment has been minimized.

Member

GeertvanHorrik commented May 30, 2018

We haven't hit any issues in our (pretty large) apps. But please do create a PR, happy to put this into 5.6 which shouldn't be too far from being released.

@GeertvanHorrik

This comment has been minimized.

Member

GeertvanHorrik commented Jun 17, 2018

Sorry I didn't get back to this, was on holidays. Were you interested in doing a PR? I am currently trying to finalize a 5.6 release.

@GeertvanHorrik GeertvanHorrik added this to the 5.6.0 milestone Jun 17, 2018

@GeertvanHorrik GeertvanHorrik added the bug label Jun 17, 2018

@GeertvanHorrik GeertvanHorrik self-assigned this Jun 17, 2018

@GeertvanHorrik GeertvanHorrik modified the milestones: 5.6.0, 5.7.0 Jul 4, 2018

@GeertvanHorrik GeertvanHorrik modified the milestones: 5.7.0, 5.8.0 Aug 7, 2018

@GeertvanHorrik GeertvanHorrik modified the milestones: 5.8.0, 5.9.0 Oct 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment