Should I always implement the dispose pattern if my class defines a field in which the field’s type implements the dispose pattern? c#

Question

I recently read this in "CLR via C#" by Jeffery Richter;

Important If a class defines a field in which the field’s type implements the dispose pattern, the class itself should also implement the dispose pattern. The Dispose method should dispose of the object referred to by the field. This allows someone using the class to call Dispose on it, which in turn releases the resources used by the object itself.

Would this be true in the following example?

public class SomeClass
{
    private readonly StreamReader _reader; //a disposable class

    public SomeClass(StreamReader reader)
    {
        _reader = reader;
    }
}

Although StreamReader is a disposable class its intance has been passed in via the constructor, it will therefore probably be referenced somewhere else and so implementing IDisposable on SomeClass so that _reader can be disposed seems like a bad idea. Is the point that Jeffery Richter is trying to make only applicable to classes where the instances of disposable classes are instantiated within that class?

You might consider a constructor that accepts a `bool closeStream` parameter so the caller can control whether or not the stream gets closed on disposal. Even in that case, I'd still implement IDisposable and in the `Dispose()` method, close the stream if the instantiator wanted the stream closed. — itsme86, Sep 01 '16 at 15:06

Yuval Itzchakov · Accepted Answer · 2016-09-01T15:08:41.487

2

Although StreamReader is a disposable class its instance has been passed in via the constructor, it will therefore probably be referenced somewhere else and so implementing IDisposable on SomeClass

That really depends. Generally, it is a good rule of thumb to implement IDisposable when you're holding disposable resources. But, if you know for a fact that someone else is going to hold a reference to the said resource, you can create an overload in your class constructor which explicitly asks the caller if he wants you to dispose:

public class SomeClass : IDisposable
{
    private readonly StreamReader _reader; //a disposable class
    private bool shouldDispose;

    public SomeClass(StreamReader reader) : this(reader, true)
    {
    }

    public SomeClass(StreamReader reader, bool shouldDispose)
    {
        _reader = reader;
        this.shouldDispose = shouldDispose;
    }

    public void Dispose()
    {
        if (shouldDispose)
        {
            Dispose(true);
        }
    }

    protected void Dispose(bool isDisposing)
    {
        if (isDisposing)
        {
            _reader.Dispose();
        }
    }
}

edited Sep 01 '16 at 15:08

answered Sep 01 '16 at 15:00

Yuval Itzchakov

146,575
32
257
321

Thanks for the response. You state that "if you know for a fact that someone else is going to hold a reference" - I don't see that the class could ever know this and therefore I should not dispose the reader in the Dispose method because something else may try to use the reader and find it has already been disposed. – mark_h Sep 01 '16 at 15:12
@mark_h The class won't know. But whoever consumes your class should know if he's passing along the reader only there, or if he's also holding a reference to the reader as well. – Yuval Itzchakov Sep 01 '16 at 15:13
I take your point, they would hold a reference to the reader, then dispose of the class they passed it into and then try to use the reader again, it wouldn't be rocket science for them to work out what went wrong, and if this is indeed best practice they should actually expect it. – mark_h Sep 01 '16 at 15:15
The idea of a "shouldDispose" switch has put my mind at rest as to how I might decide how to handle this scenario in the future. Thanks – mark_h Sep 01 '16 at 15:23

kemiller2002 · Answer 2 · 2016-09-01T15:17:39.077

There is almost never a scenario where the term "always" is appropriate. There are a lot of scenarios where this can be both true and not true. Here's the following example

public class DbSomething : IDisposable
{
    private SqlConnection _connection;

    public DbSomething (SqlConnection connection){
       _connection = connection;
    }

~DbSomething() {
     Dispose(true);
}
bool disposed = false;

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


protected virtual void Dispose(bool disposing)
{
  if (disposed)
     return; 

  if (disposing) 
  {
     _connection.Dispose();
  }
  disposed = true;
 }
}

Now if this class implements IDisposable and disposes the connection, what happens if this connection is going to be used somewhere else? This class is modifying the state of an object that doesn't belong to it.

Consequently,

public class DbSomething : IDisposable
{
    private SqlConnection _connection;

    public DbSomething (){
       _connection = new SqlConnection();
    }

    //same dispose
}

This class has control over the SqlConnection object. It created it and it should dispose of it. So what happens if you make the SqlConnection public for other things to consume?

public class DbSomething
{
    public SqlConnection Connection;

    public DbSomething (){
       Connection = new SqlConnection();
    }

   //same dispose  
}

Now I would still default to, that object created it, that object should get rid of it, but depending on the code, that might not be possible. It might be just a factory to create a long lived object which eventually needs to be disposed, and the creation object is no longer necessary after that. In this case case having the creation object dispose of it becomes a problem, so even though the first two scenarios seem like good ideas, deviating from them is the appropriate choice.

The method could also be something like this, where it doesn't even hold an instance to the object to dispose:

public class DbSomething
{
   public SqlConnection CreateSqlConnection () => return new SqlConnection();
}

Should I always implement the dispose pattern if my class defines a field in which the field’s type implements the dispose pattern? c#

2 Answers2