WeakReference is dead

Question

I am using event aggregator , using weak reference for method in my subscriber object that I want to process.

When subscribing is created a weak reference and my subscribers collection is updated accordingly. However, when I try to execute the publish event, the weak reference was cleared by the GC. Below is my code:

 public class EventAggregator { private readonly ConcurrentDictionary<Type, List<Subscriber>> subscribers = new ConcurrentDictionary<Type, List<Subscriber>>(); public void Subscribe<TMessage>(Action<TMessage> handler) { if (handler == null) { throw new ArgumentNullException("handler"); } var messageType = typeof (TMessage); if (this.subscribers.ContainsKey(messageType)) { this.subscribers[messageType].Add(new Subscriber(handler)); } else { this.subscribers.TryAdd(messageType, new List<Subscriber> {new Subscriber(handler)}); } } public void Publish(object message) { if (message == null) { throw new ArgumentNullException("message"); } var messageType = message.GetType(); if (!this.subscribers.ContainsKey(messageType)) { return; } var handlers = this.subscribers[messageType]; foreach (var handler in handlers) { if (!handler.IsAlive) { continue; } var actionType = handler.GetType(); var invoke = actionType.GetMethod("Invoke", new[] {messageType}); invoke.Invoke(handler, new[] {message}); } } private class Subscriber { private readonly WeakReference reference; public Subscriber(object subscriber) { this.reference = new WeakReference(subscriber); } public bool IsAlive { get { return this.reference.IsAlive; } } } } 

I subscribe and publish via:

 ea.Subscribe<SomeEvent>(SomeHandlerMethod); ea.Publish(new SomeEvent { ... }); 

I’m probably doing something very stupid that said that I’m afraid to see my mistake.

Answer 1

There are several problems here (others have already mentioned some of them), but the main thing is that the compiler creates a new delegate object that no one refers to. The compiler accepts

 ea.Subscribe<SomeEvent>(SomeHandlerMethod); 

and inserts the appropriate delegate transform, effectively providing:

 ea.Subscribe<SomeEvent>(new Action<SomeEvent>(SomeHandlerMethod)); 

Then later this delegate will be collected (there is only your WeakReference for him), and the subscription will be closed.

You also have security issues (I assume you are using ConcurrentDictionary for this purpose). In particular, access to ConcurrentDictionary and List not thread safe at all. Lists must be locked, and you need to use ConcurrentDictionary correctly to create updates. For example, in your current code, it is possible that two separate threads are in the TryAdd block, and one of them will not end, which will lead to the loss of the subscription.

We can fix these problems, but let me outline a solution. A weak event schema can be difficult to implement in .Net because of these automatically generated delegate instances. Instead, the Target delegate will be captured in WeakReference if it has one (this could be if it is a static method). Then, if the method is an instance method, we will build the equivalent of Delegate , which has no purpose, and therefore there will be no strong reference.

 using System.Collections.Concurrent; using System.Diagnostics; public class EventAggregator { private readonly ConcurrentDictionary<Type, List<Subscriber>> subscribers = new ConcurrentDictionary<Type, List<Subscriber>>(); public void Subscribe<TMessage>(Action<TMessage> handler) { if (handler == null) throw new ArgumentNullException("handler"); var messageType = typeof(TMessage); var handlers = this.subscribers.GetOrAdd(messageType, key => new List<Subscriber>()); lock(handlers) { handlers.Add(new Subscriber(handler)); } } public void Publish(object message) { if (message == null) throw new ArgumentNullException("message"); var messageType = message.GetType(); List<Subscriber> handlers; if (this.subscribers.TryGetValue(messageType, out handlers)) { Subscriber[] tmpHandlers; lock(handlers) { tmpHandlers = handlers.ToArray(); } foreach (var handler in tmpHandlers) { if (!handler.Invoke(message)) { lock(handlers) { handlers.Remove(handler); } } } } } private class Subscriber { private readonly WeakReference reference; private readonly Delegate method; public Subscriber(Delegate subscriber) { var target = subscriber.Target; if (target != null) { // An instance method. Capture the target in a WeakReference. // Construct a new delegate that does not have a target; this.reference = new WeakReference(target); var messageType = subscriber.Method.GetParameters()[0].ParameterType; var delegateType = typeof(Action<,>).MakeGenericType(target.GetType(), messageType); this.method = Delegate.CreateDelegate(delegateType, subscriber.Method); } else { // It is a static method, so there is no associated target. // Hold a strong reference to the delegate. this.reference = null; this.method = subscriber; } Debug.Assert(this.method.Target == null, "The delegate has a strong reference to the target."); } public bool IsAlive { get { // If the reference is null it was a Static method // and therefore is always "Alive". if (this.reference == null) return true; return this.reference.IsAlive; } } public bool Invoke(object message) { object target = null; if (reference != null) target = reference.Target; if (!IsAlive) return false; if (target != null) { this.method.DynamicInvoke(target, message); } else { this.method.DynamicInvoke(message); } return true; } } } 

And the test program:

 public class Program { public static void Main(string[] args) { var agg = new EventAggregator(); var test = new Test(); agg.Subscribe<Message>(test.Handler); agg.Subscribe<Message>(StaticHandler); agg.Publish(new Message() { Data = "Start test" }); GC.KeepAlive(test); for(int i = 0; i < 10; i++) { byte[] b = new byte[1000000]; // allocate some memory agg.Publish(new Message() { Data = i.ToString() }); Console.WriteLine(GC.CollectionCount(2)); GC.KeepAlive(b); // force the allocator to allocate b (if not in Debug). } GC.Collect(); agg.Publish(new Message() { Data = "End test" }); } private static void StaticHandler(Message m) { Console.WriteLine("Static Handler: {0}", m.Data); } } public class Test { public void Handler(Message m) { Console.WriteLine("Instance Handler: {0}", m.Data); } } public class Message { public string Data { get; set; } } 

Answer 2

The delegate object that wraps your SomeHandlerMethod backstage is probably the garbage collected between Subscribe and Publish .

Try the following:

 Action<SomeEvent> action = SomeHandlerMethod; ea.Subscribe<SomeEvent>(SomeHandlerMethod); ea.Publish(new SomeEvent { ... }); GC.KeepAlive(action); 

Perhaps the old syntax in this case is a little clear:

 Action<SomeEvent> action = new Action<SomeEvent>(SomeHandlerMethod); 

Another thing to keep in mind if your code is multi-threaded is the race condition when a subscribed event cannot be added (TryAdd may return false).

As for the solution, see atomic reaction:

 public void Subscribe<TMessage>(IHandle<TMessage> handler) { [...] public interface IHandler<T> { Handle(T event); } 

Or:

 public void Subscribe<TMessage>(Action<TMessage> handler) { [...] object targetObject = handler.Target; MethodInfo method = handler.Method; new Subscriber(targetObject, method) [...] subscriber.method.Invoke(subscriber.object, new object[]{message}); 

I do not know if a reflection MethodInfo object can be stored in WeakReference, i.e. if it is temporary or not, and if it is stored with a strong link, will it be held on an assembly containing Type (if we are talking about a dll plugin) ...

Answer 3

You pass an instance of the action in which no one contains the link, so it is immediately available to the garbage collection. Your action does contain a strong link to your instance using the method (if it is not static).

What can you do if you want to support the same API signature (you can pass in the IHandle interface and, if you want), change the Subscribe parameter as Expression, analyze it and find an instance of the Target action object and save the WeakReference value instead.

See here how to do this. Delegate actions. How to get instance calling method