Anything wrong with short-lived event handlers?

Question

For example, is there anything wrong with something like this?

private void button1_Click(object sender, EventArgs e)
{
    Packet packet = new Packet();

    packet.DataNotSent += packet_DataNotSent;

    packet.Send();
}

private void packet_DataNotSent(object sender, EventArgs e)
{
    Console.WriteLine("Packet wasn't sent.");
}

class Packet
{
    public event EventHandler DataNotSent;

    public void Send()
    {
        if (DataNotSent != null)
        {
            DataNotSent(null, EventArgs.Empty);
        }
    }
}

If it were just a simple integer variable declaration it would be fine because once it went out of scope and would later be collected by the garbage collector but events are a bit .. longer-living? Do I have to manually unsubscribe or take any sort of measures to make sure this works properly?

It just feels.. weird. Not sure if it is correct or not. Usually when I add to an event handler it lasts for the entirety of the application so I'm not sure about what happens when constantly adding and removing event handlers.

Answer 1

Technically, there is nothing wrong with this code. Since there won't be any references to packet after the method finishes, the subscription is going to be (eventually) collected too.

As a matter of style, doing this is very unusual, I think. Better solution would probably be to return some kind of result from Send() and base your next action on that. Sequential code is easier to understand.

Answer 2

That's really the beauty of event-based programming -- you don't really care who / what / if anyone is listening, you just say something happened and let whoever subscribed react accordingly.

You don't have to manually unsubscribe but you should unsubscribe as the object goes out scope. The link will keep the subscribers alive (from @pst).

Answer 3

I understand that you can get a memory leak if the event source i.e. Packet class and the event sink i.e. the handler of the event have a lifetime mismatch.

In this case the delegate that you create is scoped to this function because the event sink is only scoped to this function.

You can make a call to packet.DataNotSent -= packet_DataNotSent; after the send method executes to ensure that the delegate is garbage collected. Please read this