Who Disposes of an IDisposable public property?

Question

If I have a SomeDisposableObject class which implements IDisposable:

class SomeDisposableObject : IDisposable
{
    public void Dispose()
    {
        // Do some important disposal work.
    }
}

And I have another class called AContainer, which has an instance of SomeDisposableObject as a public property:

class AContainer
{
    SomeDisposableObject m_someObject = new SomeDisposableObject();

    public SomeDisposableObject SomeObject
    {
        get { return m_someObject; }
        set { m_someObject = value; }
    }
}

Then FxCop will insist that AContainer is also made IDisposable.

Which is fine, but I can't see how I can safely call m_someObject.Dispose() from AContainer.Dispose(), as another class may still have a reference to the m_someObject instance.

What is the best way to avoid this scenario?

(Assume that other code relies on AContainer.SomeObject always having a non-null value, so simply moving the creation of the instance outside of the AContainer is not an option)

Edit: I'll expand with some example as I think some commenters are missing the issue. If I just implement a Dispose() method on AContainer which calls m_someObject.Dispose() then I am left with these situations:

// Example One
AContainer container1 = new AContainer();
SomeDisposableObject obj1 = container1.SomeObject;
container1.Dispose();
obj1.DoSomething(); // BAD because obj1 has been disposed by container1.

// Example Two
AContainer container2 = new AContainer();
SomeObject obj2 = new SomeObject();
container2.SomeObject = obj2; // BAD because the previous value of SomeObject not disposed.
container2.Dispose();
obj2.DoSomething(); // BAD because obj2 has been disposed by container2, which doesn't really "own" it anyway.

Does that help?

Answer 1

If you have an disposable object on your class you implement IDisposable with a Dispose method that disposes wrapped disposables. Now the calling code has to ensure that using() is used or that an equivalent try / finally code that disposes the object.

Answer 2

You could just flag the Disposal in Dispose(). After all Disposal isn't a destructor - the object still exists.

so:

class AContainer : IDisposable
{
    bool _isDisposed=false;

    public void Dispose()
    {
        if (!_isDisposed) 
        {
           // dispose
        }
        _isDisposed=true;
    }
}

add this to your other class, too.

Answer 3

It really depends on who notionally "owns" the disposable object. In some cases, you may want to be able to pass in the object, for instance in the constructor, without your class taking responsibility for cleaning it up. Other times you may want to clean it up yourself. If you're creating the object (as in your sample code) then it should almost certainly be your responsibility to clean it up.

As for the property - I don't think having a property should really transfer ownership or anything like that. If your type is responsible for disposing of the object, it should keep that responsibility.

Answer 4

The real problem might be your object oriented design. If AContainer is Disposed, all its member objects should also be disposed. If not it sound like you can dispose a body but want to keep the leg instance living. Doesn't sound right.

Answer 5

The reason you cannot safely call Dispose() on AContainer's instance of SomeDisposableObject is due to lack of encapsulation. The public property provides unrestricted access to part of the internal state. As this part of the internal state must obey the rules of the IDisposable protocol, it is important to make sure that is well encapsulated.

The problem is similar to allowing access to an instance used for locking. If you do that, it becomes much harder to determine where locks are acquired.

If you can avoid exposing your disposable instance the problem of who will handle the call to Dispose() goes away as well.

Answer 6

In general, I think whoever creates the object should be responsible for Disposal. In this case, AContainer creates SomeDisposableObject, so it should be Disposed when AContainer is.

If, for some reason, you think that SomeDisposableObject should live longer than AContainer - I can only think of the following methods:

• leave SomeDisposableObject unDisposed, in which case the GC will take care of it for you
• give SomeDisposableObject a reference to AContainer (see WinForms Controls and Parent properties). As long as SomeDisposableObject is reachable, so is AContainer. That'll prevent the GC from Disposing AContainer, but if someone calls Dispose manually - well, you'd Dispose SomeDisposableObject. I'd say that's expected.
• Implement SomeDisposableObject as a method, say CreateSomeDisposableObject(). That makes it clear(er) that the client is responsible for Disposal.

All in all, though - I'm not really sure the design makes sense. After all, you seem to be expecting client code like:

SomeDisposableObject d;
using (var c = new AContainer()) {
   d = c.SomeObject;
}
// do something with d

That seems like broken client code to me. It's violating Law of Demeter, and plain ol' common sense to me.

Answer 7

There is no single answer, it depends on your scenario, and the key point is ownership of the disposable resource represented by the property, as Jon Skeet points out.

It's sometimes helpful to look at examples from the .NET Framework. Here are three examples that behave differently:

• Container always disposes. System.IO.StreamReader exposes a disposable property BaseStream. It is considered to own the underlying stream, and disposing the StreamReader always disposes the underlying stream.

• Container never disposes. System.DirectoryServices.DirectoryEntry exposes a Parent property. It is not considered to own its parent, so disposing the DirectoryEntry never disposes its parent.

  In this case a new DirectoryEntry instance is returned each time the Parent property is dereferenced, and the caller is presumably expected to dispose it. Arguably this breaks the guidelines for properties, and perhaps there should be a GetParent() method instead.

• Container sometimes disposes. System.Data.SqlClient.SqlDataReader exposes a disposable Connection property, but the caller decides if the reader owns (and therefore disposes) the underlying connection using the CommandBehavior argument of SqlCommand.ExecuteReader.

Another interesting example is System.DirectoryServices.DirectorySearcher, which has a read/write disposable property SearchRoot. If this property is set from outside, then the underlying resource is assumed not to be owned, so isn't disposed by the container. If it's not set from outside, a reference is generated internally, and a flag is set to ensure it will be disposed. You can see this with Lutz Reflector.

You need to decide whether or not your container owns the resource, and make sure you document its behavior accurately.

If you do decide you own the resource, and the property is read/write, you need to make sure your setter disposes any reference it's replacing, e.g.:

public SomeDisposableObject SomeObject    
{        
    get { return m_someObject; }        
    set 
    { 
        if ((m_someObject != null) && 
            (!object.ReferenceEquals(m_someObject, value))
        {
            m_someObject.Dispose();
        }
        m_someObject = value; 
    }    
}
private SomeDisposableObject m_someObject;

UPDATE: GrahamS rightly points out in comments that it's better to test for m_someObject != value in the setter before disposing: I've updated the above example to take account of this (using ReferenceEquals rather than != to be explicit). Although in many real-world scenarios the existence of a setter might imply that the object is not owned by the container, and therefore won't be disposed.

Answer 8

Design you have mentioned here is not something could handle this scenario. You said there is a container for that class then it should dispose it along with itself. If other objects may be using it then it is not the container and scope of you class widens and it need to dispose at boundary of that scope.

Answer 9

I'll try to answer my own question:

Avoid It in the First Place

The easiest way out of this situation is to refactor the code to avoid the problem entirely.
There are two obvious ways to do this.

External instance creation
If AContainer does not create a SomeDisposableObject instance, but instead relies on external code to supply it, then AContainer will no longer "own" the instance and is not responsible for disposing of it.

The externally created instance could be supplied via the constuctor or by setting the property.

public class AContainerClass
{
    SomeDisposableObject m_someObject; // No creation here.

    public AContainerClass(SomeDisposableObject someObject)
    {
        m_someObject = someObject;
    }

    public SomeDisposableObject SomeObject
    {
        get { return m_someObject; }
        set { m_someObject = value; }
    }
}

Keep the instance private
The main issue with the posted code is that the ownership is confused. At Dispose time the AContainer class cannot tell who owns the instance. It could be the instance that it created or it could be some other instance that was created externally and set via the property.

Even if it tracks this and knows for certain that it is dealing with the instance that it created, then it still cannot safely dispose of it as other classes may now have a reference to it that they obtained from the public property.

If the code can be refactored to avoid making the instance public (i.e. by removing the property entirely) then the issue goes away.

And If It Can't Be Avoided...

If for some reason the code cannot be refactored in these ways (as I stipulated in the question) then in my opinion you are left with some fairly difficult design choices.

Always Dispose of the instance
If you choose this approach then you are effectively declaring that AContainer will take ownership of the SomeDisposableObject instance when the property is set.

This makes sense in some situations, particularly where SomeDisposableObject is clearly a transient or subservient object. However it should be documented carefully as it requires the calling code to be aware of this transfer of ownership.

(It may be more appropriate to use a method, rather than a property, as the method name can be used to give a further hint about ownership).

public class AContainerClass: IDisposable
{
    SomeDisposableObject m_someObject = new SomeDisposableObject();

    public SomeDisposableObject SomeObject
    {
        get { return m_someObject; }
        set 
        {
            if (m_someObject != null && m_someObject != value)
                m_someObject.Dispose();

            m_someObject = value;
        }
    }

    public void Dispose()
    {
        if (m_someObject != null)
            m_someObject.Dispose();

        GC.SuppressFinalize(this);
    }
}

Only Dispose if still the original instance
In this approach you would track whether the instance was changed from the one originally created by AContainer and only dispose of it when it was the original. Here the ownership model is mixed. AContainer remains the owner of its own SomeDisposableObject instance, but if an external instance is supplied then it remains the responsibility of the external code to dispose of it.

This approach best reflects the actual situation here, but it can be difficult to implement correctly. The client code can still cause issues by performing operations like this:

AContainerClass aContainer = new AContainerClass();
SomeDisposableObject originalInstance = aContainer.SomeObject;
aContainer.SomeObject = new SomeDisposableObject();
aContainer.DoSomething();
aContainer.SomeObject = originalInstance;

Here a new instance was swapped in, a method called, then the original instance was restored. Unfortunately the AContainer will have called Dispose() on the original instance when it was replaced, so it is now invalid.

Just give Up and let the GC handle it
This is obviously less than ideal. If the SomeDisposableObject class really does contain some scarce resource then not disposing of it promptly will definitely cause you issues.

However it may also represent the most robust approach in terms of how the client code interacts with AContainer as it requires no special knowledge of how AContainer treats the ownership of the SomeDisposableObject instance.

If you know that the disposable resource isn't actually scarce on your system then this may actually be the best approach.

---

Some commenters have suggested that it may be possible to use reference counting to track if any other classes still have a reference to the SomeDisposableObject instance. This would be very useful as it would allow us to dispose of it only when we know it is safe to do so and otherwise just let the GC handle it.

However I am not aware of any C#/.NET API for determining the reference count of an object. If there is one then please let me know.

Answer 10

An interesting thing that I've encountered is that SqlCommand owns an SqlConnection (both implement IDisposable) instance usually. However calling dispose on the SqlCommand will NOT dispose the connection too.

I've discovered this also with the help of Stackoverflow right here.

So in other words it matters if the "child" (nested?) instance can/will be reused later.