3

I running my static analysis checks again and I'm getting 100s of nags on 

Label foo = new Label(); //Where this is an ASP.NET web forms label

Do I need to call dispose on these web forms labels? Also, how late in the page life cycle is it safe to call Dispose on these? If a class implements IDisposable, but doesn't actually do anything in it, is there any harm in not calling dispose?

(Yes, I have already read that closing connections is a good thing and using blocks are a good thing, too.)

DataSet is another class that seems to implement IDisposable for inexplicable reasons.

MatthewMartin
  • 32,326
  • 33
  • 105
  • 164

4 Answers4

6

Actually you do not need to call Dispose() on web form controls that you add to a parent page. I, too, saw these warnings and was trying to figure out how to handle them and before wrapping all of the control creations in using statements I tested whether or not dispose is actually called on a control that was added to a parent.

TestDispose.ascx.cs:

public partial class TestDispose : System.Web.UI.UserControl {
  public void override Dispose() {
    // Set breakpoint here
    base.Dispose();
  }
}

TestPage.aspx.cs:

public partial class TestPage : System.Web.UI.Page {
  public void override OnInit() {
    // test will be disposed of when the page is destroyed
    TestDispose test = new TestDispose();
    test.ID = "TestDispose";
    this.Controls.Add(test);

    // test1 will not be disposed of because it was not added
    // to the page's control tree.
    TestDispose test1 = new TestDispose();
    test1.ID = "TestDispose1";
  }
}

If you run this in the debugger with a breakpoint on TestDispose.Dispose() you'll find out that Dispose() is called by the parent page as the page is being destroyed. Once I determined this I started adding exclusions with the justification of 'Dispose() is called on objects within the Page's Control tree.'

Update

The one thing that bothers me is I cannot find this actually documented anywhere. While the ASP.Net Page Lifecycle is a good resource it doesn't mention calling Dispose() on child controls.

Using Reflector, however, I was able to determine that Dispose() is called through the following hierarchy:

System.Web.UI.Page.AspCompatBeginProcessRequest
-> System.Web.UI.Page.ProcessRequest
   -> System.Web.UI.Page.ProcessRequestCleanup
      -> System.Web.UI.Control.UnloadRecursive

System.Web.UI.Control.UnloadRecursive will loop through each Controls property and end up calling Dispose() for each Control object. Any object which inherits from System.Web.UI.UserControl implements IDisposable.

So, unfortunately, without actual documentation we are currently relying on an implementation detail and not a contract. While I won't change my exclusions/justifications because of this, I just wanted to bring this to light for other readers of this answer.

Community
  • 1
  • 1
Joshua
  • 8,112
  • 3
  • 35
  • 40
3

If a class implements IDisposable, but doesn't actually do anything in it, is there any harm in not calling dispose?

There is no harm in not calling dispose if the class doesn't do anything. However, you are then making an assumption that a) the implementation won't change and b) nothing is subclassing it

DataSet is another class that seems to implement IDisposable for inexplicable reasons

On the subject of DataSet itself, there is a question here on Stack Overflow that comes to the conclusion that disposing it speeds up the memory cleanup due to it being a MarshalByValueComponent.

I think a good rule of thumb would be: if you know that the implementation doesn't do anything and it's easy to dispose, then dispose it, but don't go out of your way (like setting Page.Unload event handlers) to do it. However, if you know it releases unmanaged resources, always dispose it unless the documentation says otherwise (ala SharePoint).

Community
  • 1
  • 1
Richard Szalay
  • 83,269
  • 19
  • 178
  • 237
1

You will have to look at the class carefully. If it's just a harmless Dispose() then you can ignore it. Several classes have it because of their base class.

The major argument against this reasoning is: It might change in a future version. Not likely for Label though.

But be careful, if the class (or its base class) implements the full Disposable pattern it might also have a destructor (Finalizer). And then the Dispose() is very much needed to cancel that finalizer. Not doing so would be a major loss of performance.

H H
  • 263,252
  • 30
  • 330
  • 514
0

Yes, if the Dispose() method doesn't do anything, then not calling Dispose will be harmless. If your static analysis tool is actually picking up on these you should probably turn it off if you plan to work with Winforms. IMO, it's not a good analysis since there are numerous scenarios where Dispose() is known to be chained and only the root call is necessary (i.e. streams/readers).

Kirk Woll
  • 76,112
  • 22
  • 180
  • 195