CODEWITHSUNDEEP
c#winformsdependency-injectionidisposable
SwDevMan81
SwDevMan81

Reputation: 49978

Disposing of Constructor Injected object

Lets say I have a class that associates itself with another class. It would look something like the following:

public class DisposableClassOne : IDisposable
{
   private class mDisposableClassTwo;

   public DisplosableClassOne(DisposableClassTwo dcTwoInjected)
   {
      mDisposableClassTwo = dcTwoInjected;
   }

   public void Dispose()
   {
      // Should I dispose here? or make caller dispose of dcTwoInjected
      //mDisposableClassTwo.Dispose();
   }
}

Should I call the Dispose method of mDisposableClassTwo or should I make the caller handle it like this?

using(DisposableClassTwo dcTwoInjected = new DisposableClassTwo())
using(DisposableClassOne dcOne = new DisposableClassOne(dcTwoInjected))
{
   // do stuff with dcOne
}

I'm thinking making the caller handle it is the best way to go, but I think by putting the call in the Dispose method it guarantees that it will get called. Is there a better way to handle this?

Upvotes: 3

Views: 3284

Answers (6)

Craig Brunetti
Craig Brunetti

Reputation: 595

The concept of "cleanup by owner" is far too common to be ignored. Ownership must be more than just coincidence.

If you want software that is easier to maintain, avoid side effects that may be incorrectly interpreted. The Stream vs StreamReader example is a great one... both are IDisposable, and as the caller, I'm making both, so in no way should I ever give myself a pass at disposing either of them. Doing so would violate the abstraction, tying my calling code's knowledge to the particulars of the stream class' implementation (which could change over time, academically).

The easiest answer is to have the two usings(), even if you then go and make (and document!) that the second IDisposable will dispose of the first.

Upvotes: 0

supercat
supercat

Reputation: 81159

To use IDisposable objects properly, one must follow the principle "Would the last one out please turn off the lights". There are a few nice scenarios, and an icky one:

  1. The creator of an object gives it to another object (the "recipient") but the creator of the first object controls the recipient's lifetime, and can know when the recipient will no longer need the passed object. In that case, the creator of the original object takes care of Dispose.
  2. The creator of an object gives it to the recipient, and the recipient knows that nobody else will use the object once it's been handed over. In that case, the recipient will call Dispose when it's done with the object.
  3. An object recipient may sometimes be used in ways that match #1 and sometimes in ways that match #2, but the supplier of the object will know when it's given which situation applies. This situation may be handled by telling the recipient whether or not the recipient should Dispose of the object it is given.
  4. The supplier and recipient will both want to use the object, but may have no idea which is going to outlive the other. This is the toughest situation.

I tend to like the approach suggested in #3. Sometimes situation #4 applies, though. One way of handling that is for the recipient of the object to fire an event when it's done with the object. That event can Interlocked.Decrement a counter indicating how many objects are still using the passed object. Once that counter hits zero, the object may be deleted.

Upvotes: 1

LBushkin
LBushkin

Reputation: 131676

If the class you are creating logically owns(1) the constructor injected resource then it should dispose of it internally. If it does not own the resource, then it should do nothing, and rely on the consumer to decide when it should be disposed.

If your class owns a reference to an unmanaged resource, you may also need to implement a finalizer (destructor) since there is no guarantee that anyone will ever call your Dispose method at all.

In general, you want to avoid cases where the caller must decide when they should dispose of an object passed to the constructor of a class. It's possible for the caller to dispose of the resource prematurely (or hold on to it longer than necessary). This is not always an easy thing to achieve in certain designs ... sadly, the Disposable Object pattern doesn't always compose cleanly with itself.

(1) By ownership I mean that your class controls the lifetime of the resource which it is passed and no other code holds a reference to it. If any other code (whose lifetime is not tied to the lifetime of your class) holds a reference to the resource and will use it independently, then you are not the owner of that resource.

Upvotes: 5

n8wrl
n8wrl

Reputation: 19765

I think it is a life-time decision you have to make in your design.

Is DisposableClassOne Disposable BECAUSE it references a DisposableClassTwo?

And,

Does a DisposableClassTwo have a lifetime independent of DisposableClassOne?

To me, the answers to these two questions varies in each class design. The StreamReader/Writer is a great example of the first question being 'yes' and the second 'no' - no one would expect to use a Stream inside a StreamReader once the Reader is done with it, so Reader disposes it.

But what if DisposableClassTwo was some other resource - maybe a file refernece you passed to several classes in turn to 'do something to'. In that case, you don't want it disposed until you're ready and DisposableClassOne might be long gone by then.

Upvotes: 2

Toby
Toby

Reputation: 7544

The standard I'm familiar with is to use the Disposable pattern. Have a virtual Dispose(bool disposing) method. The Dispose() method calls Dispose(true) and the finalizer calls Dispose(false).

Then, in the main disposal method:

if (disposing)
{
    // Dispose of the referenced object, as well.
}

StreamWriter & StreamReader follow this pattern. If you explicitely call Dispose(), they also dispose the underlying Stream. If you let the finalizer do your cleanup, they leave it alone.

Alternatively, you could add a property: DisposeChild, if true, dispose the child object, if not, leave it alone.

I also agree with rcravens. Under almost all circumstances, an object should be disposed within the same scope where it was instantiated.

Upvotes: 1

ShahidAzim
ShahidAzim

Reputation: 1494

It is better not to dispose any external references, those would be disposed automatically by the caller itself.

See this thread, in which it is suggested that only member variable needs to be disposed:

Finalize/Dispose pattern in C#

Upvotes: 0

Related Questions