Should we mark objects readonly if they are fields of an IDisposable class

Question

PROBLEM DESCRIPTION:

1. I have this class (FooClass) that implements IDisposable which takes another disposable (OtherDisposableClass) as an argument of its constructor.
2. I create an instance of OtherDisposableClass
3. I create an instance of FooClass and pass the instance OtherDisposableClass to it
4. The instance of OtherDisposableClass should not be mutateable so I mark it readonly
5. I can't get rid of the reference to the instance of OtherDisposableClass in my Dispose method

QUESTION

What is the best thing to do here? Should we

1. Not mark the instance of OtherDisposableClass as readonly so we can set it to 'null' in the Dispose method
2. Mark OtherDisposableClass as readonly and do not remove the reference to it ever again.
3. Something else

Please elaborate your answer. Thank you

public class FooClass : IDisposable
{
    private readonly OtherDisposableClass _disposable;
    private readonly string _imageSource;
    private readonly string _action;
    private readonly string _destination;

    private bool _isInitialized;

    public FooClass(OtherDisposableClass disposable)
    {
        _disposable = disposable;
    }
    ~FooClass() 
    {
        // Finalizer calls Dispose(false)
        Dispose(false);
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        // Not possible because _disposable is marked readonly
        _disposable = null;
    }
}

Answer 1

The thing that does the creation of OtherDisposableClass should be disposing of it... leave it alone in FooClass (you can have it readonly if you like).

Setting it to null will have no effect anyway, it will only set the reference from within FooClass to null (which is about to be disposed anyway).

EDIT

From the code given it looks like you don't even need to implement IDisposable here - this is just over complicating things. If you created an instance of OtherDisposableClass within the constructor of FooClass then you could implement IDisposable and simply call OtherDisposableClass.Dispose() within the FooClass.Dispose().

Here FooClass is created with a dependency on OtherFooClass - OtherFooClass will outlive FooClass - therefore FooClass should be GC'd first and then there will be no reference to OtherDisposableClass remaining - which will allow it to be GC'd

Answer 2

The IDisposable interface allows you to release unmanaged resources in a deterministic way. Unmanaged resources can be file handles, database connections etc. Disposing an object does not release any memory. Memory is only reclaimed by the garbage collector and you have very little control of when and how that is done. Disposing an object does not free any memory.

Still there is a connection between garbage collection and IDisposable. If your class directly controls an unmanaged resource it should release this resource during finalization. However, having a finalizer on a class has a performance impact and should be avoided if possible. FooClass does not directly control an unmanaged resource and the finalizer is not needed. However, to enforce the deterministic release of unmanaged resources it is important the FooClass.Dispose calls OtherDisposableClass.Dispose when Dispose is called explictly (when disposing is true). If Dispose is called from the finalizer you should not call methods on other managed objects because these objects may already have been finalized.

So to sum it up:

• Garbage collection and reclaiming of memory is distinct from releasing unmanaged resources.

• The Dispose method should always release unmanaged resource.

• The Dispose method should call Dispose on aggregated objects but only when called directly (not when called from the finalizer).

• It should be possible to call Dispose multiple times without any problems.

• There is no point in setting an IDisposable field to null in the Dispose method.

• Avoid implementing finalizers if you do not control any unmanaged resources directly.

• When implementing IDisposable you have to take inheritance into consideration because both the base class and the derived class may want to call Dispose on fields and/or release unmanaged resources. Sealing a class can avoid a lot of this complexity.

To elaborate on the point about setting the field to null:

I assume that your code is sane. E.g., only FooClass has a permanent reference to OtherDisposableClass and both objects will after disposal be unusable (e.g. throw ObjectDisposedException in all public methods except Dispose).

If you set the reference to OtherDisposableClass to null then that instance becomes eligible for garbage collection because there are no references to the instance. However, after disposing FooClass you surely will not keep an reference to that instance because it is unusable. This means that both the FooClass and the OtherDisposableClass are no longer reachable from a GC root and the entire object graph is now eligible for garbage collection. So there is no need to sever the link between FooClass and OtherDisposableClass because they will become eligible for garbage collection at the same time.

Answer 3

In VB.NET, if a type which implements IDisposable has any variables that are declared WithEvents, it is generally a good idea--and is sometimes very important--to set those variables to null in the Dispose method, since failure to do so will mean that objects to which those variables referred will be left holding back-references to the object that was disposed. That can cause serious memory leaks in the scenario where many short-lived objects subscribe to events from a long-lived object.

Although C# does not allow VB.NET-style WithEvents declarations, it's possible to code properties in C# which behave similarly:

// Handle a property Wobbler which identifies an object whose Wibbled event
// I want to handle with my WibbleWobble method.
Woozle _wobble;
Woozle Wobbler {
  get { return _wobbler; }
  set {
    var wasWobbler = _wobbler
    if (wasWobbler == value) return; // Don't unsubcribe and resubscribe same object
    if (wasWobbler != null)
      wasWobbler.Wibbled -= WibbleWobble; // Unsubscribe old event
    if (value != null)
      value.Wibbled += WibbleWobble; // Subscribe new event
    _wobbler = value;
  }
};

If one uses a pattern such as that (it can be a nice way to ensure subscribes get paired up with unsubscribes), one should set Wobbler to null in one's Dispose method, to ensure that any event that was attached to it gets detached.

Other than the aforementioned scenarios, it generally doesn't matter whether Dispose sets things to null, since the only thing one should be doing with the objects referred to by those fields is disposing them and cancelling their events, calling Dispose multiple times on a well-coded object should be harmless. There are a few cases, however, were setting a field to null in dispose may be helpful. Some collections store data using an array along with an indication of which slots hold meaningful data; if a collection "removes" an item from the array by indicating a slot doesn't hold anything meaningful, but doesn't null out the reference, that item and anything to which it holds a direct or indirect reference will be kept alive by the GC. If disposed objects null out their references to other objects, the amount of data kept alive by the collection may be minimized.

Incidentally, note that the code above is not thread safe, and cannot very cleanly be made thread-safe without locking, even if the subscribe/unsubscribe methods are thread safe and one uses only accesses _wobbler via Interlocked methods. If _wobbler is updated before subscriptions/unsubscriptions are processed, then if Wobbler is to some object George in one thread and to something else in another, the second thread might send George an unsubscribe request before the first thread has sent its subscription request (resulting in a dangling subscription). If it's not updated before processing subscriptions/unsubscriptions, then two simultaneous attempts to set Wobbler to George could result in George being given two consecutive subscription requests (which could eventually result in a dangling subscription).

Answer 4

You are correct in setting you member IDisposable to null in Dispose(bool disposing), see: https://msdn.microsoft.com/en-us/library/ms244737.aspx

As you cannot if the field is marked readonly the solution is I am afraid to remove the readonly.