7

Is it a good practice to pass IDisposable as a parameter to a method and dispose it inside that method. This is sort of inevitable when you have to use several threads. Well, the best practices says the owner(caller) should dispose it.

E.g. 

public void MyMethod(MyClass reader){
    using(reader){
        //some code
    }
}

What if the owner (creating thread) no longer exist? E.g. 

interface IReader : IDisposable {
    string Read();
}

public class MyReader : IReader {

    public string Read()
    {
        return "hellow world";
    }
    public void Dispose()
    {
        //dispose code
    }
}

Here you find the problem...

public void Start() {
    MyReader[] readerSet = new MyReader[5];
    for (int i = 0; i < readerSet.Length; i++) {
        readerSet[i] = new MyReader();
    }
    foreach (IReader reader in readerSet) {
        ThreadPool.QueueUserWorkItem(new WaitCallback(Run), reader);
    }
    //exit after creating threads
}


public void Run(Object objReader) {
    IReader reader = (IReader)objReader;
    using (reader) { 
    //use the reader
    }
}
Daniel Daranas
  • 21,689
  • 9
  • 60
  • 108
Sriwantha Attanayake
  • 6,943
  • 4
  • 37
  • 44

4 Answers4

6

I think you would be better off taking a creation delegate to guarantee disposal of the object.

Example

public void Start() {
    var makeReader = new Func<IReader>(() => new MyReader()); 
    for (int i = 0; i < 5; i++) {
        ThreadPool.QueueUserWorkItem(Run, makeReader);
    }
}    

public void Run(Object state) {
    var makeReader = (Func<IReader>)state;
    using (var reader = makeReader()) { 
        //use the reader
    }
}
knocte
  • 14,579
  • 6
  • 67
  • 110
ChaosPandion
  • 73,399
  • 16
  • 113
  • 152
  • Well so far so good... but how can you call this method. E.g. where are you going to call MyClass constructor? You can't have Func(MyClass) or Func(new MyClass()). – Sriwantha Attanayake Sep 03 '11 at 08:56
  • That's what I call brilliant. superb! exact thing I was looking for. – Sriwantha Attanayake Sep 03 '11 at 17:03
  • why do you cast? if you don't mind I'm going to improve the answer to make it more statically typed, so there's no need to cast – knocte Oct 26 '13 at 15:57
  • oops, sorry, reverted my change; I guess you need to cast because you're using QueueUserWorkItem, and for that, Run() needs to comply with a very simple delegate signature – knocte Oct 26 '13 at 16:00
2

No, the owner should dispose it. The owner is usually an object that created IDisposable instance in the first place. You can read about IDisposable best practices here.

Do transitively dispose of any disposable fields defined in your type from your Dispose method. You should call Dispose() on any fields whose lifecycle your object controls. For example, consider a case where your object owns a private TextReader field. In your type's Dispose, you should call the TextReader object's Dispose, which will in turn dispose of its disposable fields (Stream and Encoding, for example), and so on. If implemented inside a Dispose(bool disposing) method, this should only occur if the disposing parameter is true—touching other managed objects is not allowed during finalization. Additionally, if your object doesn’t own a given disposable object, it should not attempt to dispose of it, as other code could still rely on it being active. Both of these could lead to subtle-to-detect bugs.

It may be a good idea to minimize passing IDisposable instances around so that you don't have to think about owners too much.

Dmitry
  • 15,996
  • 2
  • 37
  • 65
  • *'It may be a good idea to minimize passing IDisposable instances around*' ...I totally disagree. Have you looked at the `IObserver/IObservable` implementation? The `Unsubscriber` is an IDisposable. How would you pass a Stream into XmlWriter or any of the other Stream sub-classes without passing the IDisposable stream? The best advice is to use the `using` clause as @Chaos demonstrates. – IAbstract Sep 02 '11 at 23:02
  • Nothing wrong with passing around IDisposables; simply make sure you have a solid lifecycle management strategy and don't just start calling dispose when you think you might be done with something. – Rex M Sep 02 '11 at 23:22
  • minimize: to reduce or keep to a minimum. In my opinion code is more readable when you don't have to worry about the owner. Of course there is no way to avoid passing IDisposable in all cases. But why not make the code simpler if this is possible? – Dmitry Sep 02 '11 at 23:28
  • One always has to worry about the owner, but in most cases it shouldn't be hard to know who the owner is. In some sense, the question that needs to be asked when passing a parameter is whether one is giving the object to the callee, or is one merely letting the callee "borrow" it. – supercat Sep 03 '11 at 20:02
0

That entirely depends on your specific situation.

Generally, the “owner” should dispose the object, but it's your job to figure out who that is. It might not be the creator and it might not be the caller in your case either.

svick
  • 214,528
  • 47
  • 357
  • 477
0

At any moment in time from the moment of its creation to its Dispose call, any instance of IDisposable should have precisely one owner who will be responsible for deleting it. Typically, the owner of an IDisposable will be the entity that created it, but there are cases where it may be helpful to have an object hand off ownership of an IDisposable to another object. Consider, for example, the following two hypothetical classes:

  1. SoundSource, which implements a GetAudio method that returns the next 'n' samples of a sound. SoundSource implements IDisposable because it can be used to stream audio from a file; if it's not properly disposed, the file won't get closed.
  2. AsyncSoundPlayer, which implements a PlaySound method that accepts a SoundSource and starts it playing, aborting any previous playing sound.

In many situations, the routine which loads a SoundSource won't really care when it gets done playing; it's apt to be more convenient to have the playback routine take ownership of the SoundSource. Note that in some cases the routine that requests playback might want to keep ownership of the SoundSource. The best approach is to allow a means by which the caller of a routine can specify whether it wishes to keep or hand off ownership of a passed-in IDisposable.

supercat
  • 69,493
  • 7
  • 143
  • 184
  • @knocte: One approach is to have the method accept a Boolean along with the IDisposable indicating whether ownership is being transferred. A little crude, but better than assuming that ownership will always be transferred or never be transferred. Nicer would be to have a wrapper for the `IDisposable` class which behaved just like the "real" instance, but with a do-nothing `Dispose` method; unfortunately, there's no general way to implement such a thing without having to code it for each class. – supercat Oct 26 '13 at 18:46
  • well, I like @ChaosPandion solution – knocte Oct 26 '13 at 19:48
  • @knocte: If object creation can be deferred until one has entered the only scope where an object will be needed, the solution you like is a good one, since object ownership need never leave the scope. Such an approach seems to fit the original poster's needs, but isn't applicable in all cases. – supercat Oct 29 '13 at 00:12
  • true, but passing a boolean along with the IDisposable is the ugliest approach I could think of; first, because it's just ugly design, second because it would be not very clear when there are other parameters in the method, and third, because I think that maybe it's much more elegant to use an attribute on the parameter, which I think that C# allows, i.e.: SomeMethod([SomeAttrib] someParam, someOtherParam) – knocte Oct 29 '13 at 16:03
  • @knocte: Booleans are a bit ugly, though one can use named-parameter syntax in a function call to make their meaning clear (e.g. `SetPicture(myPicture, passOwnership := true);`. Using differently-named methods may sometimes be nicer, but nested function calls become awkward. If my control stores a passed-in picture in a nested object which has separate `SetPictureTakingOwnership` and `SetPictureShared` methods, those methods have to be called from largely-redundant routines. By contrast, using a flag, my control can call `nestedControl.SetPicture(myPicture, passOwnership);` – supercat Oct 29 '13 at 16:27
  • @knocte: I wouldn't be particularly opposed to using an `enum`, but if e.g. one method uses a `PictureOwnershipMode` and another uses an `ImageOwnershipMode`, conversion between the type may be awkward. – supercat Oct 29 '13 at 16:29