0

I've heard that you are supposed to unsubscribe from events on controls that get disposed.

TextBox textBox;
void Initialize()
{
    textBox = new TextBox();
    textBox.Click += ClickHandler;

    this.Disposed += DisposeControl;
}

void ClickHandler(object sender, EventArgs e)
{
    this.foo = bar;
}

void DisposeControl(object sender, EventArgs e)
{
    textBox.Click -= ClickHandler;
    this.Disposed -= DisposeControl; // DOES THIS LINE MAKE SENSE?
}

There's no reference to the control after it has been disposed so I would assume there is no need to unregister any event handlers. Yet, as I mentioned people are saying that you're supposed to. Does that include unregistering the dispose event itself?

user875234
  • 2,399
  • 7
  • 31
  • 49
  • 1
    I found *it makes sense*. But I think it is useless since `Disposed` should be called only once. –  Nov 13 '19 at 17:45
  • 1
    You can check the relevance here: [Component.Dispose](https://referencesource.microsoft.com/#System/compmod/system/componentmodel/Component.cs,5657bad357bf9d75,references) –  Nov 13 '19 at 17:47
  • 1
    @Peter Duniho Please don't link to four generally related questions and say my question is a duplicate. Is that how this works? You just google four semi-related questions and call it a duplicate if the question sounds like it must have been answered at some point in the past? I don't see my exact question addressed in any of those four questions. – user875234 Nov 13 '19 at 19:35
  • I agree that I think SO's "close as duplicate" process is a touch aggressive. I have found that it's not always easy to see the similarities when you're in the thick of an issue. In this case, while the articles @PeterDuniho linked to don't answer your question in the exact way that you posed it, they do answer your general question. The 4th one above is the one I linked to in my answer. The only difference in my answer is that I "connected the dots" between your example and what was expressed in those other answers. – jwatts1980 Nov 13 '19 at 19:56

2 Answers2

1

I prefer use IDisposable to handle this case

List<IDisposable> eventsToDispose = new List<IDisposable>();

eventsToDispose.Add(Disposable.Create(() => 
{
     textBox.Click += ClickHandler;
}));

Then you can dispose handler

eventsToDispose.ForEach(o => o.Dispose());
junlan
  • 302
  • 1
  • 5
1

To answer you question "DOES THIS LINE MAKE SENSE?", in your example it does not. That is because the event source and target are both the same. I am going to rewrite your code a bit.

class SOTest1 : Control
{
    private bool bar = true;
    private bool foo { get; set; }

    TextBox textBox;
    void Initialize()
    {
        textBox = new TextBox();
        textBox.Click += ClickHandler;

        this.Disposed += DisposeControl;
    }

    void ClickHandler(object sender, EventArgs e)
    {
        this.foo = bar;
    }

    void DisposeControl(object sender, EventArgs e)
    {
        textBox.Click -= ClickHandler;
        this.Disposed -= DisposeControl; // DOES THIS LINE MAKE SENSE?
    }
}

In this case, since Control implements IDisposable which calls the Disposed event at the end of the disposal, both the object with the event and the event handler are the same object. So, the C# garbage collector is going to see that the object and all references have been disposed and will clean it all up.

Also, since textBox and its event handlers are contained within that object, the textBox.Click -= ClickHandler is also redundant, as it will all be cleaned up by the GC when the SOTest1 object is disposed.

It's not a bad practice to clean up event subscribers, but, it does add code that is unnecessary much of the time. C# does a great job of cleaning up resources when your code has finished with them.

There are some cases where memory leaks can happen, like if the lifecycle of the object with the handler is shorter than the lifecycle of the object with the event. The main answer in this article has a good example of that case: What best practices for cleaning up event handler references?

jwatts1980
  • 7,254
  • 2
  • 28
  • 44