5

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.

Ryan Peschel
  • 11,087
  • 19
  • 74
  • 136
  • where are you detaching the eventhandler from the event? and why you want to attach the same event handler every time you click the button? – Ashley John Sep 27 '11 at 01:37
  • You know you can make the event just an action instead of EventHandler? – Ritch Melton Sep 27 '11 at 01:39
  • http://stackoverflow.com/questions/298261/do-event-handlers-stop-garbage-collection-from-occuring http://stackoverflow.com/questions/506092/is-it-necessary-to-explicitly-remove-event-handlers-in-c – CharithJ Sep 27 '11 at 01:41
  • if this is the usage of the Packet class, I would be inclined to change the Send() method to return the result of the Send operation and get rid of the event handler. However, I suppose you might want to make the Send() operation asynchronous, in which case you would need a delegate to perform a callback. – Igor Pashchuk Sep 27 '11 at 01:42

6 Answers6

6

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.

svick
  • 236,525
  • 50
  • 385
  • 514
  • Could you elaborate on the second part of your answer? I'm always open to alternatives and I agree with you on the style of this being a bit flimsy. Would you do something like having Send returning a boolean (or enum, etc.) indicating whether or not the send was successful and handle it right there? The problem with returning a value is that I'm looking for this to be asynchronous and I feel trapped in to using events. – Ryan Peschel Sep 27 '11 at 01:52
  • @RyanPeschel, if you want to to things asynchronously (and you're not using C# 5), then using either events or callback delegates is probably your best option. And yeah, I would return `bool` if I needed to indicate failure. Or some custom type if I needed more data. – svick Sep 27 '11 at 02:03
  • 1
    Take a look at the new Tasks pattern. I Think it really is a clean way of handling both synchronous and asynchronous scenarios. ( http://www.codethinked.com/net-40-and-systemthreadingtasks ) – Polity Sep 27 '11 at 02:22
  • @Polity: Thank you for the great article! It was very readable and well-written. I am already using Tasks but I found it informative, nonetheless. The problem with returning values from tasks is that it involves blocking until the value returns (might as well use synchronous code! well, almost). – Ryan Peschel Sep 27 '11 at 02:36
  • @RyanPeschel, with tasks, there is another option – using `ContinueWith()`. – svick Sep 27 '11 at 02:46
  • 1
    @Ryan Peschel: That is not true. A task means nothing more than "some work". How or when the work is executed is outside the reach of the task. Executing the task might be async or sync, thats up to the work inside the task and the way you handle the tasks. For a good read and a comprehensive view of tasks, please read: http://www.codeproject.com/KB/cs/TPL1.aspx – Polity Sep 27 '11 at 02:47
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).

Austin Salonen
  • 49,173
  • 15
  • 109
  • 139
  • 1
    This is slightly misleading -- the "*link*" will *keep* the subscriber alive. –  Sep 27 '11 at 01:37
1

When the object that publishes the event becomes eligible for garbage collection, all the objects subscribed to that event become eligible for garbage collection as well (provided there are no other references to them).

So, in your example:

  • When packet goes out of scope, it becomes eligible for GC.
  • So the object containing packet_DataNotSent become eligible for GC as well (unless referenced by something else).
  • If the object containing packet_DataNotSent is referenced by something else, it will of course not be GC-ed, but it will still automatically "unsibscribe" when packet is GC-ed.
Branko Dimitrijevic
  • 50,809
  • 10
  • 93
  • 167
1

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

smoothe
  • 568
  • 1
  • 4
  • 11
1

I guess you've simplified the example for brevity, but in your snippet, there is no need to use an event. Packet.Send() could just return a result that is checked. You only really need a more complicated approach if there is going to be some asynchronicity (e.g. an asynchronous operation, scheduling for future execution, etc.), and Packet.Send() does not return immediately.

In terms of object life-cycle management, your event subscription does not pose a problem, since an event subscription will keep the handler alive but not vice versa. That is, the class that button1_Click belongs to will not be garbage collected while there is a live reference to a packet that it has subscribed to. Since the packets have a short lifespan, this won't be a problem.

If in your real-world usage, Packet.Send() cannot return the result, then I would be tempted to pass a delegate to the packet rather than subscribe to an event on it, assuming only one object needs to be notified of the failure.

Ergwun
  • 12,579
  • 7
  • 56
  • 83
0

Events should be used for scenarios where one doesn't know who might be interested in being notified when something happens or some action becomes necessary. In your example, if anyone's interested in knowing when the packet is discovered to be undeliverable, the code calling the constructor is apt to know who that would be. It would thus be better to have the packet's constructor accept an Action<Packet, PacketResult> delegate than publish an event for that purpose.

supercat
  • 77,689
  • 9
  • 166
  • 211