2

I'm working on an OSS project to make the popular MediaInfo library easier to use in .NET, but this question is generalizable.

If a derived class D always instantiates an object O when calling its base class DB's constructor. DB sets its value to that sent to its constructor, but the value itself is declared in DB's base class B:

  1. Who "owns" O (AKA mediaInfo in the code below)?
  2. In the case of a .NET application, which of these should implement IDisposable? Note: O is unmanaged, or at least is an instantiation of a managed object wrapped around an unmanaged library, but does need cleanup in the form of "MediaInfo.Close();". I am not certain this counts as "unmanaged."

To help clarify, let me use actual code:

D derives from DB:

// MediaFile is "D" 
public sealed class MediaFile : GeneralStream
{
    public MediaFile(string filePath)
        : base(new MediaInfo(), 0) {
        // mediaInfo is "O"
        mediaInfo.Open(filePath);
    }
}

DB sets its inherited O, derived from B:

// GeneralStream is "DB"
public abstract class GeneralStream : StreamBaseClass
{
    public GeneralStream(MediaInfo mediaInfo, int id) {
        this.mediaInfo = mediaInfo; // declared in StreamBaseClass
        // ...
    }
}

B declares O:

// StreamBaseClass is "B"
public abstract class StreamBaseClass
{
    protected MediaInfo mediaInfo; // "O" is declared
    // ...
}
Charles Burns
  • 10,310
  • 7
  • 64
  • 81
  • The different `Stream` types already implement `IDisposable` - this means any class inheriting from one of them inherits this implementation. – Oded Mar 11 '12 at 18:28
  • @Oded, I think the streams in the question are something different – they don't inherit from `System.IO.Stream`. – svick Mar 11 '12 at 19:06
  • @svick - I agree, but thought I would point out that the BCL `Stream` and related types do implement the interface. – Oded Mar 11 '12 at 19:17

3 Answers3

2

The object, which holds a reference to the resource, owns it.

StreamBaseClass has the reference mediaInfo and should implement IDisposable. The reference and the Dispose method will automatically be inherited by the deriving classes.

Olivier Jacot-Descombes
  • 104,806
  • 13
  • 138
  • 188
  • Possession of a reference to a resource does not imply ownership. Possession of the *only* reference to a resource does imply ownership (if nobody else holds a reference, nobody else can clean it up) but there are many cases where resources may be shared among several different class objects; if the first of those objects to be `Disposed` were to call `Dispose` on the resource, it would disrupt anyone else using it. – supercat Mar 12 '12 at 23:26
  • Yes, however here Charles shows us a class hierarchy. This involves several classes, but not necessarily several objects. – Olivier Jacot-Descombes Mar 13 '12 at 13:53
  • The base class knows that it's receiving a `MediaInfo` in its constructor, but unless the contract for the constructor explicitly declares that it's assuming ownership of that resource, neither it nor the caller should assume that it should be responsible for taking care of the passed-in resource. The constructor which accepts a resource as a parameter is public, so while it may in this particular case be invoked as a chained call, that isn't always going to be the case. – supercat Mar 13 '12 at 14:51
1

If a class C owns a variable which is a non-exposed local variable V which implements IDisposable, then C should be IDisposable and C's IDisposable should dispose V.

If a class D owns a native resource N, then D should be IDisposable (which deletes N) and should also have a finalizable destructor which calls Dispose() on itself to release N.

If you follow this pattern, then if you ever have an IDisposable, you should always Dispose() it when you're done and that will delete everything all the way down the object tree; but also if someone forgets to you (read: a colleague, user of your library etc) won't leak any objects because the native resource will be cleaned up by D's finalizer.

SecurityMatt
  • 6,593
  • 1
  • 22
  • 28
1

The responsibility of an IDisposable belongs to the object that creates it, in the absence of any explicit agreement otherwise. Contrary agreements are generally used in cases where the creator of a resource may have no idea of the consumer's lifetime. I would suggest that in many cases when a constructor or factory method is producing what may be the last consumer of a passed-in IDisposable, that method should accept a parameter indicating whether it should accept responsibility for calling Dispose, or else accept a callback delegate which will, if non-null, be invoked when the consumer no longer needs the object. If the creator of the object will outlive the consumer, it can pass null; if the creator will have no use for the object once it's been handed off, it can pass the object's Dispose method. If the creator doesn't know whether it will outlive the consumer, it can pass a method which will determine whether the object is still needed and call Dispose if not.

With regard to your particular case, constructing an IDisposable within a chained constructor call is a recipe for resource leaks (since there's no way to wrap chained constructor calls in a try-finally block). If you were to somehow handle that safely (e.g. using a factory method rather than a chained constructor, or by using a [threadstatic] hack), I would suggest that since the creator of the object (the derived class) is going to know the lifetime of the consumer (the base class), ownership and cleanup responsibility should stay with the object creator.

supercat
  • 77,689
  • 9
  • 166
  • 211
  • So, supercat suggests IDisposable be implemented by the creator and Olivier Jacot-Descombes suggests it should be the reference owner. I was hoping there would be a clear "right" answer, but you both have en impressive reputation and seem to disagree. Does it help that in my particular case, the unmanaged object is essentially immutable? While I haven't enforced this in code yet, the use model is that a GeneralStream is instantiated and the derived MediaInfo is never changed. – Charles Burns Mar 13 '12 at 00:53
  • 1
    @Charles: At any given time, from the moment an `IDisposable` is created until it is destroyed, there should be some object or scoping block which can be identified as being responsible for ensuring that `Dispose` will gets called, either by taking care of it itself, or having some other object or scoping block promise to take care of it. In many cases there's nothing wrong with having an object accept an `IDisposable` in its constructor and agree in its contracts to call `Dispose` on it when no longer needed, but such assumption of responsibility should be documented, not guessed at. – supercat Mar 13 '12 at 06:44
  • I've taken your suggestion (I think) and altered the code so that StreamBaseClass both holds the reference and implements IDisposable. The file path which the unmanaged portion opens is now passed up through two constructors to StreamBaseClass, which seems a little inelegant, but has actually reduced code size since the base class takes care of some initialization that was formerly handled in each derived class. Thank you for the suggestion! – Charles Burns Mar 13 '12 at 15:34