Docs: Consider simplifying the Dispose pattern

[svick]

The article Implementing a Dispose method says that types that implement the IDisposable interface should implement the dispose pattern:

```c#
class BaseClass : IDisposable
{
// Flag: Has Dispose already been called?
bool disposed = false;
// Instantiate a SafeHandle instance.
SafeHandle handle = new SafeFileHandle(IntPtr.Zero, true);

// Public implementation of Dispose pattern callable by consumers.
public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}

// Protected implementation of Dispose pattern.
protected virtual void Dispose(bool disposing)
{
if (disposed)
return;

  if (disposing) {
     handle.Dispose();
     // Free any managed objects here.
     //
  }

  disposed = true;

}
}

class DerivedClass : BaseClass
{
// Flag: Has Dispose already been called?
bool disposed = false;
// Instantiate a SafeHandle instance.
SafeHandle handle = new SafeFileHandle(IntPtr.Zero, true);

// Protected implementation of Dispose pattern.
protected override void Dispose(bool disposing)
{
if (disposed)
return;

  if (disposing) {
     handle.Dispose();
     // Free any other managed objects here.
     //
  }

  disposed = true;
  // Call base class implementation.
  base.Dispose(disposing);

}
}

A lot of this code is only there to make it easier to have a derived class that has a finalizer:

```c#
class DerivedClassWithFinalizer : BaseClass
{
   // Flag: Has Dispose already been called?
   bool disposed = false;

   // Protected implementation of Dispose pattern.
   protected override void Dispose(bool disposing)
   {
      if (disposed)
         return; 

      if (disposing) {
         // Free any managed objects here.
         //
      }

      // Free any unmanaged objects here.
      //
      disposed = true;

      // Call the base class implementation.
      base.Dispose(disposing);
   }

   ~DerivedClass()
   {
      Dispose(false);
   }
}

But since finalizers should almost never be used, I think it would make sense to change the recommended pattern, to make creating a base class that implements IDisposable easier. The new pattern would look like this:

```c#
class BaseClass : IDisposable
{
// Flag: Has Dispose already been called?
bool disposed = false;
// Instantiate a SafeHandle instance.
SafeHandle handle = new SafeFileHandle(IntPtr.Zero, true);

// Public implementation of Dispose pattern callable by consumers.
public virtual void Dispose()
{
if (disposed)
return;

  handle.Dispose();
  // Free any other managed objects here.
  //

  disposed = true;

}
}

class DerivedClass : BaseClass
{
// Flag: Has Dispose already been called?
bool disposed = false;
// Instantiate a SafeHandle instance.
SafeHandle handle = new SafeFileHandle(IntPtr.Zero, true);

// Protected implementation of Dispose pattern.
public override void Dispose()
{
if (disposed)
return;

  handle.Dispose();
  // Free any other managed objects here.
  //

  disposed = true;
  // Call base class implementation.
  base.Dispose();

}
}

class DerivedClassWithFinalizer : BaseClass
{
// Flag: Has Dispose already been called?
bool disposed = false;

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

// Protected implementation of Dispose pattern.
protected virtual void Dispose(bool disposing)
{
if (disposed)
return;

  if (disposing) {
     // Free any managed objects here.
     //
  }

  // Free any unmanaged objects here.
  //
  disposed = true;

  // Call the base class implementation.
  if (disposing)
     base.Dispose();

}

~DerivedClassWithFinalizer()
{
Dispose(false);
}
}
```

This makes creating a derived class with a finalizer harder, but it also makes both base classes and derived classes without a finalizer simpler and shorter. I think such tradeoff is well worth it.

Also, while the article says that the version of the pattern without a finalizer is recommended, it still makes writing a finalizer sound like a reasonable alternative. Maybe most of the text and code related to finalizers should be moved to another article, or deleted altogether?

Another place where the Dispose pattern is discussed is in the Framework Design Guidelines. That is a content that's not open source and can't be changed. Would it be possible to delete that article instead?

cc: @rpetrusha (author of the article), @jnm2 (https://github.com/dotnet/csharplang/issues/1451#issuecomment-389580012), @davkean (https://twitter.com/davkean/status/994546618014707712)

[sharwell]

I would be more in favor of the pattern update if the following was added with analyzer enforcement:

Do not add a finalizer to a derived type when the base IDisposable implementation does not follow the Dispose(bool) pattern.

[svick]

@sharwell Personally, I don't really care what will be the recommended pattern if you do want to use finalizers (if there even is one), as long as:

• The main article clearly says you shouldn't use finalizers. (Certainly more clearly than it does now.)
• The main article talks about finalizers as little as possible.

So, I would be fine with what you're suggesting.

[mairaw]

@davkean any thoughts on this proposal? can you help updating this topic perhaps? 😉

[BillWagner]

@svick I added a link to the content article above in the description. Can you verify that the link is correct?

[jeremyVignelles]

I like that recommendation, is there anything new here?

We had a few discussions about the correct pattern here and there

[Bosch-Eli-Black]

This makes creating a derived class with a finalizer harder, but it also makes both base classes and derived classes without a finalizer simpler and shorter.

@svick Does it just make creating a derived class with a finalizer harder, or does it make it totally impossible? (And if it makes it impossible, then what would the workaround be?)

Thanks 🙂

[sharwell]

Does it just make creating a derived class with a finalizer harder, or does it make it totally impossible?

It makes it harder, but not impossible. I believe @jnm2 identified the pattern that would work:

public class Base : IDisposable {
  public virtual void Dispose() { }
}

public class DerivedAddsFinalizerSupport : Base {
  public sealed override Dispose() {
    Dispose(true);
    GC.SuppressFinalize(this);
  }

  protected virtual void Dispose(bool disposing) {
    if (disposing) {
      base.Dispose();
    }
  }
}

[jnm2]

Yes. DerivedAddsFinalizerSupport would then also have the finalizer calling Dispose(false) as usual.

So it's not impossible, but even if it was impossible, there would still be the workaround of composition: Introduce a new internal class that contains a finalizer and put an instance of it in a readonly field of the derived class, and encapsulate the state that needs finalization inside that internal class rather than putting things that need finalization directly in the derived class.

This second option has two benefits: it avoids shipping a public type with a finalizer, which is not recommended, and when possible you can have the new internal class derive from SafeHandle rather than declaring your own finalizer. (And declaring your own finalizer is still not recommended!)

This second option has one downside: you have an additional allocation per instance of the derived class. (Trading safety for fewer allocations this makes sense if measurements show that it's not a premature thing to do.)

[Bosch-Eli-Black]

Awesome! 🙂

My personal two cents is that I would love to see the docs reorganized so that:

• The main documentation about IDisposable focuses on things like how to implement IDisposable using public void Dispose(), how the using statement works, etc. and avoids discussion of things such as finalizers and SafeHandle.
• There's a separate document detailing all of the complexity of finalizers, for those poor souls who still have to deal with them 🙂