Messenger and references

Nov 17, 2010 at 7:18 PM
Edited Nov 17, 2010 at 11:29 PM

Hi there,

I'm curious about the design of the messenger: it holds weak references to the receipient objects, but not to the actions themselves. This means that if you do the obvious thing inside a class:

Messenger.Default.Register<SomeMessage>(this, ShowDialog);

where ShowDialog is an instance method defined as:

private void ShowDialog(SomeMessage msg)
{
}

... the Action<SomeMessage> delegate that's held (in a strong reference) by the Messenger class itself retains a strong reference to the target instance, meaning that object is never garbage-collected unless the handler is specifically unregistered.

I think that it would be useful to have an overload of Register that delegates to an Action<TRecipient, TMessage>, where TRecipient is cast from the WeakReference that is currently held; then we could do this:

Messenger.Default.Register<MyClass, SomeMessage>(this, (myClassInstance, msg) => myClassInstance.ShowDialog(msg));

And avoid the problem.

Thanks,

Ben

Coordinator
Nov 17, 2010 at 8:52 PM

Hi,

This is an interesting point. I added an item in my backlog to look into this and come up with something to answer this concern.

Cheers,

Laurent

Nov 18, 2010 at 6:47 PM

Hi Laurent,

That's good news -- I think it's such a common usage that many people will run into this problem.

Ben

Jan 6, 2011 at 3:59 PM

Hi,

Ben is right, the Action holds a strong reference to the subscribing class. This prevents the garbage collection action's owner class, causing a memory leak.

I have used the following code snippet to demonstrate the issue:

Running the program shows that the GC collects the first client, but the second client object is still alive because the action creates a strong reference to it. Removing the act variable permits the collection of the second client, so the weak reference will be NULL in the CheckWref method.

Anyway I think your did an excellent job with this framework, it is a great and truly a lightweight solution. Thank you!

Paul

 

    class Program
    {
        private static WeakReference wref;
        private static Action act;

        static void Main(string[] args)
        {
            MakeRefs();
            CheckWref();
           
            Console.ReadLine();
        }

        public static void MakeRefs()
        {
            Client c1 = new Client("FirstClient");
            Client c2 = new Client("SecondClient");

            wref = new WeakReference(c2);
            Action act = new Action(c2.DoIt);
        }

        public static void CheckWref()
        {
            GC.Collect();
            if (wref.Target != null)
            {
                Client c = wref.Target as Client;
                act();
            }
            else
            {
                Console.WriteLine("wref is null");
            }
        }

 

    class Client
    {
        public Client(string name)
        {
            Name = name;
        }

        ~Client()
        {
            Console.WriteLine(Name + " finalized");
        }

        public string Name
        {
            get;
            set;
        }

        public void DoIt()
        {
            Console.WriteLine(string.Format("{0}.DoIt();", Name));
        }
    }

 

 

Coordinator
Jan 6, 2011 at 8:27 PM

Great work investigating. Thanks for that. I took good note.

Cheers,

Laurent

Mar 2, 2011 at 8:55 PM

This is actually the exact problem I am having. Due to the structure of the Silverlight app I am working on, it is a little bit hard to know exactly when an appropriate time to unregister is. The register is actually being done in an entity, and some things would have to move around to be able to call unregister at the appropriate time. Is there a way around this? Can I explicitly create a WeakAction, or something like that?

Other than this issue, I have had great success with this toolkit. Many thanks!

--Morgan

Mar 2, 2011 at 9:18 PM
Edited Mar 2, 2011 at 9:24 PM

Morgan,

This is the workaround I created -- an extension method for the Messenger class that uses a WeakReference to the target object in a closure that calls a static handler method. (Example usage below).

public static class MessengerExtensions
{
    public static void Register<TRecipient, TMessage>(this Messenger messenger, TRecipient recipient, object token, Action<TRecipient, TMessage> finalHandler)
        where TRecipient : class
    {
        if (recipient == null)
        {
            throw new ArgumentNullException("recipient");
        }
    
        if (finalHandler == null)
        {
            throw new ArgumentNullException("finalHandler");
        }
    
        if (messenger == null)
        {
            throw new ArgumentNullException("messenger");
        }
    
        // We can't use the recipient parameter in the closure, since that would simply create another strong
        // reference -- the situation we're trying to avoid. And since MVVM Light doesn't pass the recipient to 
        // the message handler, we have no option but to create a WeakReference to it here and use that in the closure.
        var weakRef = new WeakReference(recipient);
    
        messenger.Register<TMessage>(recipient, token,
            msg =>
            {
                var r2 = weakRef.Target as TRecipient;
    
                if (r2 == null)
                {
                    // This shouldn't happen, since the fact that the Messenger is calling this method
                    // means the recipient is still alive. Still, I don't feel right ignoring the possibility.
                    // In addition, there's no great advantage to throwing an exception here -- so we simply assume 
                    // the Messenger will unregister this closure when this method returns.
                    return;
                }
    
                finalHandler(r2, msg);
            });
    }
}    

Example Usage:

Messenger.Default.Register<RecipientClass, MessageClass>(this, null, (_this, msg) => _this.Handler(msg));

Note that if you opt to create the static handler as a lambda, as I've done above, it should not close over any target object member variables (otherwise a strong reference will be created, and the problem will remain).

Mar 2, 2011 at 9:57 PM

Fantastic! That's exactly where my mind was going. Saved me some time :)

--Morgan

Mar 9, 2011 at 10:25 PM

I noticed this problem too and I've put together a minimal test harness with some cases:

- a class that registers with a simple lambda
- a class that registers with a lambda with a reference to "this"
- a class that registers with a named delegate

Then I tried to instantiate these classes and unregister them in a few different ways:

- no unregistration at all
- "untyped" unregistration, i.e. Messenger.Default.Unregister(this)
- "typed" unregistration, i.e. Messenger.Default.Unregister<Message>(this) where I only specify the type of message
- "named" unregistration, i.e. Messenger.Default.Unregister<Message>(this, MethodName) where I specify the type of the message and the name of the function. 

Result:

- the simple lambda expression doesn't create any leak even without unregistration
- the lambda with reference to this needs the typed unregistration
- the named delegate needs the typed or the named unregistration.

This is a bit surprising to me because I thought there would be no difference between the different types of unregistration.

Here are the test classes so that you can experiment:

    public class Message
    { }

    // just a common base class
    public class MyViewModelBase : ViewModelBase
    {
        public void UntypedCleanup()
        {
            Messenger.Default.Unregister(this);
        }

        public void TypedCleanup()
        {
            Messenger.Default.Unregister<Message>(this);
        }

        public virtual void NamedCleanup() { }
    }

    // simple lambda
    public class LambdaVM : MyViewModelBase
    {
        public LambdaVM()
        {
            Messenger.Default.Register<Message>(this, m => Console.WriteLine("test"));
        }
    }
    
    // lambda with reference to this
    public class LambdaThisVM : MyViewModelBase
    {
        public LambdaThisVM()
        {
            Messenger.Default.Register<Message>(this, m => Console.WriteLine(this.GetHashCode()));
        }
    }

    // named delegate
    public class DelegateVM : MyViewModelBase
    {
        public DelegateVM()
        {
            Messenger.Default.Register<Message>(this, OnMessage);
        }

        private void OnMessage(Message m)
        {
            Console.WriteLine(this.GetHashCode());
        }

        public override void NamedCleanup()
        {
            Messenger.Default.Unregister<Message>(this, OnMessage);
        }
    }

And this is the test harness:

        public enum Cleanup
        {
            None,    // no cleanup
            Untyped, // Messenger.Unregister(this)
            Typed, // Messenger.Unregister<Message>(this)
            Named    // Messenger.Unregister<Message>(this, OnMessage)
        }

        public void TestAll()
        {
            var survives = EscapesGC<LambdaVM>(Cleanup.None); // false (OK)
            survives = EscapesGC<LambdaVM>(Cleanup.Untyped);  // false (OK)
            survives = EscapesGC<LambdaVM>(Cleanup.Typed);    // false (OK)

            survives = EscapesGC<LambdaThisVM>(Cleanup.None);    // true (LEAK)
            survives = EscapesGC<LambdaThisVM>(Cleanup.Untyped); // true (LEAK)
            survives = EscapesGC<LambdaThisVM>(Cleanup.Typed);   // false (OK)

            survives = EscapesGC<DelegateVM>(Cleanup.None);    // true (LEAK)
            survives = EscapesGC<DelegateVM>(Cleanup.Untyped); // true (LEAK)
            survives = EscapesGC<DelegateVM>(Cleanup.Typed);   // false (OK)
            survives = EscapesGC<DelegateVM>(Cleanup.Named);   // false (OK)
        }

        private bool EscapesGC<T>(Cleanup cleanupMode) where T : MyViewModelBase
        {
            var vm = Activator.CreateInstance<T>();
            var reference = new WeakReference(vm);

            if (cleanupMode == Cleanup.Untyped)
                vm.UntypedCleanup();
            else if (cleanupMode == Cleanup.Typed)
                vm.TypedCleanup();
            else if (cleanupMode == Cleanup.Named)
                vm.NamedCleanup();

            vm = null;
            GC.Collect();
            GC.WaitForPendingFinalizers();
            return reference.IsAlive;
        }

Mar 27, 2012 at 2:28 PM
Edited Mar 27, 2012 at 2:29 PM

Am I missing something or is this bug still not fixed in the current MVVM light version?

Coordinator
Mar 27, 2012 at 8:38 PM

I didn’t push the fix out yet, you are correct. It is coming.

From: Pauli7 [email removed]
Sent: Tuesday, March 27, 2012 3:29 PM
To: laurent@galasoft.ch
Subject: Re: Messenger and references [mvvmlight:235068]

From: Pauli7

Am I missing something or is this bug still not fixed in current MVVM light version?

Mar 28, 2012 at 8:13 AM
Edited Mar 28, 2012 at 8:20 AM

perfect, thank you.

one question: do you think it is possible to get a preview version today? that would be great since i'm currently working on this problem.

if not, could you at least tell me which way you went?

- OpenInstanceDelegate a la Catel (http://blog.catenalogic.com/post/2011/12/29/Using-true-weak-actions-without-causing-memory-leaks.aspx)
--> major restrictions in silverlight

- Proxy a la SimpleMVVM (http://blog.tonysneed.com/2011/03/22/building-a-leak-proof-eventing-model/)
--> a bit cumbersome in usage

- Interface a la Caliburn Micro
--> not enough (at least in my situation)

regards,
stephan

OpenInstanceAction
Coordinator
Mar 28, 2012 at 1:52 PM

I went with something like Catel, though I did it first ;) and also I support more scenarios than he does.

I can pass you a preview of the DLLs. Please email me your Live ID at Laurent (at) galasoft (dot) ch.

Cheers,

Laurent

Mar 28, 2012 at 2:39 PM

thanks laurent, i sent you an email...

May 23, 2012 at 7:00 PM

Is it released?

Jun 11, 2012 at 8:37 AM

Hi Laurent,

Thanks for your fantastic framework!!

We also have memory leak because of this bug.
Could you please let me know if this bug is  fixed? In which version?

Thanks

Arjen

Jul 2, 2012 at 7:09 AM

Hi Laurent,

same problem here. Will it bit fixed soon? Could I have those preview DLLs?

Thanks

 

Tom

 

Jul 2, 2012 at 5:07 PM

18 months after I first reported it, I'm shocked that this obvious, addressable memory leak is still not fixed in the release version of this product.

I realize that MVVM light is free, but where is the professional responsibility?

Coordinator
Jul 2, 2012 at 6:05 PM

Hi,

The RTM version of the DLLs available on Codeplex and Nuget fixes the issue. Please test it and report.

It was not an easy bug to fix, mostly due to the differences in different platforms (Silverlight security makes the WPF fix impossible to apply, so testing was really tough.

Ben, the solution you suggested back then is not applicable, and I had to work with quite a few people to find a fix. It is not an easy thing to do because it involves reflection. Keeping the action in a WeakReference does not work (the action will not be executed if it is kept in a WeakReference). The solution I have now works well in all the scenarios I tested and I will write an article about the changes. While I understand your frustration, I want to underline that MVVM Light is not just free but it is also open source, so the code is out there for you to download and fix where needed.

One thing I decided in order to avoid long waits like this one is to turn around versions faster in the future. I hope that this will help avoiding frustrations.

Cheers,

Laurent

Jul 2, 2012 at 7:09 PM

Laurent,

That is good to hear.

Not that I expected you to use it, because it's not terribly clean, but how is the solution I posted "not applicable"?

I assume you don't mean that it doesn't solve the problem. :-)

Ben

Coordinator
Jul 2, 2012 at 8:27 PM

Hi Ben,

Sorry, what I meant is that my goal was to solve the issue with the existing registration methods. Your proposed solution was adding a new way to do work but was not solving the issue with the existing registration methods. I really didn't want to leave people with the old memory leak, which is why I embarked on the crusade to solve it for the existing methods.

You are right I should probably have offered your workaround earlier. Sorry for that.

Thanks,

Laurent

Jul 2, 2012 at 8:45 PM

Laurent,

Ah, of course -- it's great to hear you've solved it for existing usage, and I now understand why it took some time.

Thanks again,

Ben

Jul 2, 2012 at 9:04 PM

Thanks for you quick answer. I will try it tomorrow at work and will report my results :-).

Jul 4, 2012 at 8:51 AM

The leak seems to be closed for our software with the RTM version.

 

Thanks!

Jul 16, 2012 at 12:59 PM
Edited Jul 16, 2012 at 3:10 PM

Hi, we've encountered this problem recently too. It's great that you've done the work to fix this issue but having a quick look at the source code it looks like the token passed to Register() is stored as a hard reference still. I suspect it wouldn't be too big a job to change this to a weak reference so I'm going to try it myself and let you know if it sorts the problem. Also I notice that it will still hold a hard reference to the delegate in SL if the method is not public.

ETA: I've changed WeakActionAndToken to store a weak reference to the token too (changed Token property to WeakToken) and I also changed all my delegates to be public and now the memory leak has gone away. I'll send over my modifications and you can decide if you want to incorporate them. It might be a good idea to make it optional to make the token a weak reference as this actually broke some of my other code as it happened to be the only thing holding a hard reference to that particular object.

Coordinator
Jul 16, 2012 at 3:26 PM

That is a very good point you make about the token. I usually think of tokens as simple values but indeed it could be an object. I will update the code like your propose. Out of curiosity, how do you handle the case where the token is not alive anymore?

Regarding the SL private methods, it is correct that I am still using the old leaky implementation in that case. I will write documentation about this case. Unfortunately, the non-leaky way is impossible for private methods in SL due to security restrictions in the reflection API. As far as I can say after consulting many people who are much more clever than me (wink to Bart de Smaet amongst others), there is no other way to do that.

Thanks!

Laurent

Jul 16, 2012 at 4:43 PM

Fair point about the private methods in SL, I didn't know this was the case so it would be great to make this a prominently documented "feature". That or possibly raise an exception when a private method is used in SL?

I am using heavy objects as my tokens as it happens to suit my needs (it's the root object of a given instance of a "form" in my app) and I've been trying to avoid refactoring all the calling code to get around this. All I did to make it work was replace Token in WeakActionAndToken with a WeakReference called WeakToken and everywhere where it's accessed just did the check (item.WeakToken != null && item.WeakToken.Target != null). I appreciate that I've brough this on myself to some degree as my token would keep the weak delegate reference alive too. Use a weak reference is no problem in this instance as long as you make sure you don't rely on the Messenger to keep your object alive.

 

Nov 7, 2013 at 2:23 AM
Hi,

I've created an attached property for a Window that registers to receive a message and then closes the relevant window when it gets that message. This is done by registering a lambda expression as the callback in the static functions. In an older version (4.1.27.0) it caused the issue https://mvvmlight.codeplex.com/workitem/7579. I updated to the latest version (4.2.30.0) and now the message is never received because the Action is only referenced by the messaging library which holds it in a weak reference.

To demonstrate I reproduced the issue in a simple unit test:
using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using GalaSoft.MvvmLight.Messaging;

namespace KiMe.Accounts.Testing
{
    class TestMessage
    {
    }

    class TestResult
    {
        public bool Called;

        public void Callback()
        {
            Called = true;
        }
    }

    [TestClass]
    public class MvvmLightTests
    {
        [TestMethod]
        public void MessengerWeakReferenceIssue()
        {
            var myReference = new TestResult();
            Register(myReference);

            GC.Collect();

            Messenger.Default.Send(new TestMessage());

            Assert.IsTrue(myReference.Called);
        }

        private static void Register(TestResult reference)
        {
            Messenger.Default.Register<TestMessage>(reference, (msg) => reference.Callback());
        }
    }
}
Reading this topic I guess that's the designed behaviour, but it's not particularly obvious in this use-case so I thought it might be worth mentioning.

Cheers,

Matt.
Nov 28, 2013 at 12:02 AM
Has this fix been included in the latest release?

Thanks,
Nathan