14

Where to call Dispose() for IDisposable objects owned by an object?

public class MyClass
{
    public MyClass()
    {
        log = new EventLog { Source = "MyLogSource", Log = "MyLog" };
        FileStream stream = File.Open("MyFile.txt", FileMode.OpenOrCreate);
    }


    private readonly EventLog log;
    private readonly FileStream stream;

    // Other members, using the fields above
}

Should I implement Finalize() for this example? What if I do not implement anything at all? Will there be any problems?

My first thought was that MyClass should implement IDisposable. But the following statement in an MSDN article confused me:

Implement IDisposable only if you are using unmanaged resources directly. If your app simply uses an object that implements IDisposable, don't provide an IDisposable implementation.

Is this statement wrong?

Andrej Adamenko
  • 1,650
  • 15
  • 31
  • Acc. to your MSDN-update, you _are_ using unmanaged resources directly. You own them by holding them as fields, hence you are responsible to dispose them. If you only use them (f.e. passed as an object to a method) you should not implement `IDisposable`. – Tim Schmelter Sep 29 '14 at 10:55
  • I agree the MSDN quote is misleading. It talks about "your app" and an "app" (whatever that is) can't implement IDisposable. Only classes can. – adrianm Sep 29 '14 at 11:00
  • 4
    @TimSchmelter: The code is not using unmanaged resources *directly*; it is acquiring and using managed resources [managed resource wrappers which will in theory get cleaned up, eventually, if abandoned] which in turn hold unmanaged resources. Microsoft's terminology surrounding resources is pretty sloppy. – supercat Sep 29 '14 at 18:06

3 Answers3

27

If MyClass owns an IDisposable resource, then MyClass should itself be IDisposable, and it should dispose the encapsulated resource when Dispose() is called on MyClass:

public class MyClass : IDisposable {
    // ...
    public virtual void Dispose() {
        if(stream != null) {
            stream.Dispose();
            stream = null;
        }
        if(log != null) {
            log.Dispose();
            log = null;
        }
    }
}

No, you should not implement a finalizer here.

Note: an alternative implemention might be something like:

private static void Dispose<T>(ref T obj) where T : class, IDisposable {
    if(obj != null) {
        try { obj.Dispose(); } catch {}
        obj = null;
    }
}

public virtual void Dispose() {
    Dispose(ref stream);
    Dispose(ref log);
}
Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • if the design is as you have suggested and MyClass implements IDisposable, then shouldn't finalizer also be implemented in the same MyClass for the users who do not call Dispose. It will help preventing the memory leak or I am missing something. Is finalization only for unmanaged resource clean up. – Mrinal Kamboj Sep 29 '14 at 10:47
  • Thank you for you answer. I am confused by MSDN documentation, see my update to the question, is MSDN wrong? – Andrej Adamenko Sep 29 '14 at 10:49
  • 2
    @MrinalKamboj yes, you are missing something. Finalizers should **only** touch unmanaged resources. They **must not** touch managed resources, because the collection order is non-deterministic : the `stream` / `log` objects may have already been disposed. It is **only** the dispose that needs to do something here. Frankly, it is **incredibly** rare that you **ever** need to add a finalizer. – Marc Gravell Sep 29 '14 at 10:49
  • 1
    @AndrejAdamenko wow, yes, that guidance is completely wrong; that sounds like it has been copy/pasted from the "finalizer" page. For `IDisposable`, it comes down to: *who owns the lifetime*? In this case, it is definitely `MyClass`. – Marc Gravell Sep 29 '14 at 10:51
  • @AndrejAdamenko the .NET 4 guidance does not have this error: http://msdn.microsoft.com/en-us/library/system.idisposable(v=vs.100).aspx – Marc Gravell Sep 29 '14 at 10:53
  • @ Marc Gravell, thanks, so it looks like calls involving PInvoke, ComInterop would be ideal candidates for the finalizer, as they go beyond managed boundaries – Mrinal Kamboj Sep 29 '14 at 10:54
  • @MrinalKamboj that depends: does the type have a direct handle to unmanaged data? Note that a lot of things use safe-handle, which hides a lot of that overhead – Marc Gravell Sep 29 '14 at 10:56
  • @ Marc Gravell, not having implemented the finalizer ever in real implementation, got me confused with theory, as you said its rare. Thanks for the details – Mrinal Kamboj Sep 29 '14 at 11:20
  • What if I do not implement or do not call `MyClass.Dispose()` in my example? Will the resources be eventually freed by `GC`? – Andrej Adamenko Sep 29 '14 at 11:24
  • @AndrejAdamenko yes, they would - indeed, by GC. – Marc Gravell Sep 29 '14 at 11:33
  • @MarcGravell You should also handle the case that something goes wrong in the constructor. In this case, nothing is calling `Dispose`. I'd wrap it in a `try`/`catch` that `Dispose`s before rethrowing the exception. – Tim S. Sep 29 '14 at 12:11
  • Why did you surround the `Dispose` call with a `try/catch` block in your second example and not in your first one? In general, a `Dispose` method should not throw exceptions. – Luca Cremonesi Oct 02 '14 at 12:06
  • @Luca in general they shouldn't - in reality some do (WCF being a prime example) - as for the difference: "i just did". Pick and choose whether you want to try/catch here or not – Marc Gravell Oct 02 '14 at 12:10
1

For objects containing other IDisposable objects, it's a good and recommended practice to implement IDisposable on your own object, so others consuming your type can wrap it in a using statement:

public class MyClass : IDisposable
{
    public MyClass()
    {
        log = new EventLog { Source = "MyLogSource", Log="MyLog" };
        FileStream stream = File.Open("MyFile.txt", FileMode.OpenOrCreate);
    }


    private readonly EventLog log;
    private readonly FileStream stream;

    public void Dispose()
    {
        Dispose(true);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            // Free managed objects here
            stream.Dispose();
        }
    }

    // Other members, using the fields above
}

In your case, you aren't freeing up any managed resources so no finalizer is needed. If you were, then you would implement a finalizer and call Dispose(false), indicating to your dispose method that it is running from the finalizer thread.

If you don't implement IDisposable, you're leaving it up to the GC to clean up the resources (e.g, close the Handle on the FileStream you've opened) once it kicks in for collection. Lets say your MyClass object is eligible for collection and is currently in generation 1. You would be leaving your FileStream handle open until the GC cleans up the resource once it runs. Also, many implementations of Dispose call GC.SuppressFinalize to avoid having your object live another GC cycle, passing from the Initialization Queue to the F-Reachable queue.

Yuval Itzchakov
  • 146,575
  • 32
  • 257
  • 321
  • Thank you for you answer. I am confused by MSDN documentation, see my update to the question, is MSDN wrong? – Andrej Adamenko Sep 29 '14 at 10:51
  • Wouldn't FileStream allocate an unmanaged resource internally. It will open a FileHandle, doesn't that require clean up if user of MyClass doesn't call the Dispose. – Mrinal Kamboj Sep 29 '14 at 10:51
  • @MrinalKamboj It does. As i've said, if you dont call `Dispose`, it will leave it up to implementation of `FileStream`'s finalizer to run and close the open handle. – Yuval Itzchakov Sep 29 '14 at 10:53
  • @AndrejAdamenko I don't think that guideline is relevant, and im not sure it's even up to date. – Yuval Itzchakov Sep 29 '14 at 10:55
  • 1
    @YuvalItzchakov the crazy thing is: that dodgy guidance wasn't in the .NET 4.0 version; I think someone incorrectly copy/pasted the "finalizer" guidance into the `IDisposable` page. Sigh. – Marc Gravell Sep 29 '14 at 10:58
  • @Yuval Itzchakov got it thanks, Even FileStream would have a finalizer to take care of the unmanaged memory – Mrinal Kamboj Sep 29 '14 at 11:19
1

Much of the advice surrounding Dispose and Finalize was written by people who expected Finalize to be workable as a primary resource cleanup mechanism. Experience has shown such expectation to have been overly optimistic. Public-facing objects which acquire any sort of resources and hold them between method calls should implement IDisposable and should not override Finalize. If an object holds any resources which would not otherwise get cleaned up if it was abandoned, it should encapsulate each such resource in a privately-held object which should then use Finalize to clean up that resource if required.

Note that a class should generally not use Finalize to clean up resources held by other objects. If Finalize runs on an object that holds a reference to the other object, one of several conditions will usually apply:

  • No other reference exists to the other object, and it has already run Finalize, so this object doesn't need to do anything to clean it up.
  • No other reference exists to the other object, and it hasn't yet run Finalize but is scheduled to do so, so this object doesn't need to do anything to clean it up.
  • Something else is still using the other object, so this object shouldn't try to clean it up.
  • The other object's clean-up method cannot be safely run from within the context of a finalizer thread, so this object shouldn't try to clean it up.
  • This object only became eligible to run Finalize because all necessary cleanup has already been accomplished, so this object doesn't need to do anything to clean things up.

Only define a Finalize method in those cases where one can understand why none of the above conditions apply. Although such cases exist, they are rare, and it's better not to have a Finalize method than to have an inappropriate one.

supercat
  • 77,689
  • 9
  • 166
  • 211