Messenger and references

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:

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!

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!

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).

publicstatic class MessengerExtensions
{
publicstaticvoid Register<TRecipient, TMessage>(this Messenger messenger, TRecipient recipient, object token, Action<TRecipient, TMessage> finalHandler)
where TRecipient : class
{
if (recipient == null)
{
thrownew ArgumentNullException("recipient");
}
if (finalHandler == null)
{
thrownew ArgumentNullException("finalHandler");
}
if (messenger == null)
{
thrownew 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);
});
}
}

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).

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.

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.

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.

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.

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.

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.

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.