3

Earlier today I ran into CA1063 when running code analysis on some code at work.

I have two questions:

  1. Why does the following code not cause CA1063 even though it clearly violates some of the demands (for example Dispose is overridden)

  2. What is the actual problem with the code that caused the complicated scheme for having a virtual Dispose(bool) that is called by a sealed Dispose() and the Finalizer and so on....

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace ConsoleApplication1
{
  class Foobar : IDisposable
  {
    public Foobar()
    {
      Console.Out.WriteLine("Constructor of Foobar");
    }

    public virtual void Dispose()
    {
      Console.Out.WriteLine("Dispose of Foobar");
      GC.SuppressFinalize(this);
    }

    ~Foobar()
    {
      Console.Out.WriteLine("Finalizer of Foobar");
    }
  }

  class Derived : Foobar
  {
    public Derived()
    {
      Console.Out.WriteLine("Constructor of Derived");
    }

    public override void Dispose()
    {
      Console.Out.WriteLine("Dispose of Derived");
      GC.SuppressFinalize(this);
      base.Dispose();
    }

    ~Derived()
    {
      Console.Out.WriteLine("Finalizer of Derived");
    }
  }

  class Program
  {
    static void Main()
    {
      Console.Out.WriteLine("Start");
      using (var foo = new Derived())
      {
        Console.Out.WriteLine("...");
      }
      Console.Out.WriteLine("End");
    }
  }
}
Sarien
  • 6,647
  • 6
  • 35
  • 55
  • I've blogged about this http://msmvps.com/blogs/peterritchie/archive/2013/01/20/the-dispose-pattern-as-an-anti-pattern.aspx Basically the complicated dispose pattern it to support freeing unmanaged resources. With .NET, you have everything you need to never have to deal with unmanaged resources so the "complicated" dispose pattern is effectively obsolete (IMO). – Peter Ritchie Mar 11 '13 at 19:43
  • Since I am also using C++ CLI and a lot of native code that may not exactly apply to my situation. :) – Sarien Mar 11 '13 at 20:05
  • @PeterRitchie I have read your post and it seems the fundamental underlying reason is just "people forget to call dispose". Is that so? – Sarien Mar 11 '13 at 20:21
  • implementing `IDisposable` or the disposable pattern does stop people from forgetting to call dispose. If by "complex" you simply mean implementing `IDisposable` (CA1063 implies the Disposable Pattern), then my blog post may not really apply. `IDisposable` with the `using` statement provides an ability to have deterministic disposal in light of exceptions. – Peter Ritchie Mar 11 '13 at 20:41
  • See also http://stackoverflow.com/questions/615105/what-is-idisposable-for and http://stackoverflow.com/questions/4042519/question-on-idisposable – Peter Ritchie Mar 11 '13 at 20:42
  • Just the interface and the using support are great but that would have worked for unmanaged and managed resources and I still don't see the need for the IDisposable pattern. – Sarien Mar 11 '13 at 20:46

1 Answers1

5

Originally, Microsoft expected that many types of objects would encapsulate both managed and unmanaged resources, and that even if a particular inheritable class did not encapsulate any unmanaged resources, a class which derived from it might do so. Even though such thinking was largely wrong (it's usually much better to segregate unmanaged resources into their own objects, which may then be used as managed resources), the pattern that was designed to deal with arbitrarily-mixed managed and unmanaged resources became an established precedent.

Even though parts of the full Dispose pattern are silly, a proper simplification wouldn't leave out a whole lot. The cleanup code should be in a protected virtual method, so as to allow derived classes to add their own logic but still chain to the parent-class method; if that method is given the name Dispose, it must have a signature distinct from that of a parameterless Dispose method [though my own preference would be a parameterless method with a different name]. My biggest complaint with Microsoft's pattern is that it requires every derived class to have its own logic to protect against repeated disposal; it would have been much cleaner to have the base class take care of that in the non-virtual Dispose implementation.

supercat
  • 77,689
  • 9
  • 166
  • 211
  • "if that method is given the name Dispose, it must have a signature distinct from that of a parameterless Dispose method" Why can't it be parameterless? Wouldn't that end up being exactly what I did in my example? – Sarien Mar 11 '13 at 21:16
  • @Sarien: Having a base-class non-virtual method which chains to a virtual method makes it possible for the base class to ensure that derived-class code gets called within a wrapper. Microsoft doesn't include in their wrapper some of the things which IMHO should be there (e.g. redundant-dispose prevention) but the technique is sound. The `SuppressFinalize` call should probably be the *last* thing that's done in the disposal process, and as such belongs in the non-virtual dispose method. – supercat Mar 11 '13 at 21:23
  • But wouldn't the derived classes still have to call the superclass dispose functions? (Unless maybe using some reflection tricks are applied.) I don't see how it matters when we call SuppressFinalize. But I am probably missing a ton of exception and thread issues or something. :) – Sarien Mar 11 '13 at 21:38
  • @Sarien: The derived classes do indeed have to call the superclass `Dispose` function, although if the base-class virtual method sets a flag, the non-virtual wrapper could squawk if that flag isn't cleared by the time the derived method returns. As for what `SuppressFinalize` is called, it typically won't matter, but if an exception prevents cleanup from finishing, `SuppressFinalize` generally shouldn't be called even if the base class cleanup was completed. – supercat Mar 11 '13 at 21:45
  • I agree, using a flag to just throw when Dispose hasn't been called when the Finalizer is running seems like it would have been simpler and just as effective. – Sarien Mar 11 '13 at 21:49
  • @Sarien: Having the `Dispose` wrapper throw if it regains control before the proper flag gets set would allow the state of the object and calling context to be examined to see what happened. An "alarm bell" finalizer may be useful for finding failures to call `Dispose`, but won't fire until much later. By the time the alarm is triggered, it may be hard to ascertain when the object *should* have gotten disposed. – supercat Mar 11 '13 at 21:53
  • That's true but if Dispose isn't called I don't see how your approach helps. :) Admittedly, it does help in case Dispose is called but not properly propagated. Damn, now this is almost as complicated as the MS pattern again. :) – Sarien Mar 11 '13 at 21:56
  • 1
    @Sarien: My complaint isn't that the MS pattern is excessively complicated, but rather that it is designed to facilitate the implementation of a style of class which people should almost never write (something that combines managed and unmanaged resources). – supercat Mar 11 '13 at 22:04
  • A proper simplification would be as follows: 1) don't ever directly include unmanaged pointers in a class; instead hold the pointer in a class implementing `SafeHandle` which already implements the disposing pattern correctly; 2) don't use the disposing pattern yourself, just implement `IDisposable.Dispose`; 3) unless a class is sealed, the `Dispose` method should be virtual (an implementer might add more disposable resources). – Mike Rosoft Oct 14 '21 at 12:38