Today I would like to spend some words about this code:

public class Consumer
{
    private IThing _field;
    private IProducer _producer;

    public Consumer(IProducer producer)
    {
        _producer = producer;
        _producer.MyEvent += HandleMyEvent;
        _field = new Thing();
    }

    private void HandleMyEvent() {
        _field.DoSomething();
    }
}

There are a lot of problems with this code, so let’s analyze them one by one.

TL;DR

While it is fine for simple projects, in more complex code bases I prefer to move event subscription to dedicated method like Start and unsubscribe in a symmetric Stop (pick your preferred name, of course). This has the following advantages:

1. reduce the possibility to introduce hidden memory leaks

2. protection from in-stack events

3. better API exposed to Consumer’s clients

   1. from which derives a better understanding of object lifecycle

4. ability to stop and restart event subscription

In stack events

Consider the following implementation of the IProducer interface:

public class Producer : IProducer
{
    private EventHandler<EventArgs> _myEvent;

    public event EventHandler<EventArgs> MyEvent
    {
        add
        {
            _myEvent += value;
            _dataProduced?.Invoke(this, EventArgs.Empty);
        }
        remove
        {
            _dataProduced -= value;
        }
    }

    public void ProduceData()
    {
        _dataProduced?.Invoke(this, EventArgs.Empty);
    }
}

What happens here? By using add, the Producer will immediately invoke our handler. The reason why it could doing it may vary a lot: e.g. it wants to send the last data snapshot received. But let’s focus on the effects: our Consumer will error because HandleMyEvent is executed in stack, before the constructor of Consumer completed; our _field is still uninitialized, and HandleMyEvent with error with a NullReferenceException.

This may seem something the coder overlooked, and has a simple fix. Here it is:

public Consumer(IProducer producer)
{
    _producer = producer;
    _field = new Thing();
    _producer.MyEvent += HandleMyEvent;
}

This way the error is no longer thrown. Easy-peasy, right? Well, it’s just a rough patch, and there is more to it.

First, we are potentially executing code when our consumer is not fully constructed; as we saw, we need to reorder the constructor’s code to work it around. While the fix is easy, the problem is at semantic level.

Secondly, we are breaking the Producer abstraction. The fact that Producer implementation may change overtime, must not be a concern for Consumer. Also, there is no explicit syntax to highlight that the event can fire in subscription stack; if we’re lucky, only a few comment lines.

We may complain that the Producer should not send events in-stack and it is its fault, but let’s think again: Consumer is relying upon the implementation of Producer class to work properly, and thus we are breaking the IProducer abstraction.

Third, and most important, by moving the event subscription at the end of the constructor we are implicitly saying: “Hey, Consumer has a construction process divided in two ways: the actual object construction and the object start (or init, or whatever), where we set things up for event consumption. Those two phases are clearly distinct and the second must come before the first”. We are hiding this concept behind the constructor, while we should explicitly expose it with a Start method (or Init, or whatever). Let’s park this Start concept for a while and let’s see other problems of subscribing events in the constructors.

Unsubscribe event

At a certain point, Consumer must unsubscribe the event. If we registered the event on our constructor, then it would make sense to unsubscribe it in the Dispose method.

using System;

namespace MyNamespace
{
    public class Consumer : IDisposable
    {
        private IThing _field;
        private IProducer _producer;

        public Consumer(IProducer producer)
        {
            _producer = producer;
            _field = new Thing(); // we now know it must not BOOM
            _producer.MyEvent += HandleMyEvent;
        }

        private void HandleMyEvent() {
            _field.DoSomething();
        }

        public void Dispose() {
            _producer.MyEvent -= HandleMyEvent;
        }
    }
}

We now unsubscribed, so everything’s better now, right? Well, it depends: usually is ok, but what if our clients need to stop and restart the subscription using the same Consumer instance? Then Dispose doesn’t suit, as its semantic is to prepare the object for garbage collection, and we need to come up with something like a pair of Start and Stop methods.

Something like this:

public class Consumer
{
    private IThing _field;
    private IProducer _producer;

    public Consumer(IProducer producer)
    {
        _producer = producer;
        _field = new Thing();
    }

    private void HandleMyEvent() {
        _field.DoSomething();
    }

    public void Start() {
        _producer.MyEvent += HandleMyEvent;
    }

    public void Stop() {
        _producer.MyEvent -= HandleMyEvent;
    }
}

We may also want to ensure the Start method is called only once by introducing a _started flag, or perhaps also expose the Start method from an IStartable interface, or abstract base class, that Consumer implements/overrides, but that’s outside the scope of this article.

Memory leak

And finally: yes, that code is prone to memory leaks. Let’s take a look at this client code:

public void DoSomething() {
    [...]
    var consumer = new Consumer();
    // wooo I forgot to Dispose
    [...]
}

What happens? When DoSomething execution is over, the GC will look at our Consumer and will collect it, right? Well, no, it will see there is still a reference from Producer to Consumer and won’t collect our consumer instance. But we have no longer a reference to consumer! That is a memory leak.

We might say it is something we overlooked and we need to fix it. Yes, but before fixing it we have to know there is a problem. If we’re lucky, our PR reviewer will notice it, otherwise this memory leak will live well hidden in our code and who knows when it will be noticed. Most probably in production, as usual 😭.

If we instead exposed event subscription in a Start method, with the code above the developer would have noticed something was not working properly, i.e. Consumer not receiving events, and then palmed his face with a: “Right, I should have started the Consumer”. Hooray, bug discovered earlier and no memory leak introduced.

Conclusion

Little things may make a lot of difference to the quality of our software and make life easier for other developers, especially in complex systems.

To the next bite!