WeakReference is dead

Question

I am toying with an event aggregator using a weak reference to the method in my subscriber object I wish to handle the event.

When subscribing the weak reference is created successfully and my subscribers collection is updating accordingly. When I attempt to publish an event however, the weak reference has been cleaned up 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 am probably doing something very daft, that said I am struggling to see my error.

Answer 1

There are a few issues here (others have mentioned some of them already), but the primary one is that the compiler is creating a new delegate object that no one is holding a strong reference to. The compiler takes

ea.Subscribe<SomeEvent>(SomeHandlerMethod);

and inserts the appropriate delegate conversion, giving effectively:

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

Then later this delegate is collected (there is only your WeakReference to it) and the subscription is hosed.

You also have thread-safety issues (I'm assuming you are using ConcurrentDictionary for this purpose). Specifically the access to both the ConcurrentDictionary and the Lists are not thread-safe at all. The Lists need to be locked and you need to properly use ConcurrentDictionary to make updates. For example, in your current code, it is possible that two separate threads are in the TryAdd block and one of them will fail causing a subscription to be lost.

We can fix these problems, but let me outline the solution. The weak event pattern can be tricky to implement in .Net because of those automatically generated delegate instances. What will do instead is capture the delegate's Target in a WeakReference, if it has one (It may not if it is a static method). Then if the method is an instance method we will construct an equivalent Delegate that has no Target and thus 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 a 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 behind the scenes is probably 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 is a bit clearer in this case:

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

Another thing to watch out for if your code is multithreaded is the race condition where a subscribed event might not be added (TryAdd can return false).

As for a solution, see atomaras answer:

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 don't know if the reflection MethodInfo object could be stored in a WeakReference, i.e. if it is temporary or not, and if it's stored strongly referenced whether or not it'll hold on to the assembly containing the Type (if we're talking about a dll-plugin)...

Answer 3

You are passing in an instance of an Action that noone keeps a strong reference to so it's immediatelly available for Garbage Collection. Your action does hold a strong reference to your instance with the method though (if it's not static).

What you can do if you want to maintain the same API signature (you have the option of passing in an IHandle interface also if you want) is change the Subscribe parameter to be an Expression, parse it and locate the instance of the Target object of the Action and keep a WeakReference to that instead.

See here on how to do it Action delegate. How to get the instance that call the method