5

In a couple of places, people have suggested to use private void Dispose(bool) for the IDisposable pattern. This seems outdated though (at least for unsealed classes), as the new suggested pattern (according to Microsoft) is protected virtual void Dispose(bool).

The thing is, Code Analysis does not report private void Dispose(bool) for violating CA1063, even though it seems to violate the pattern directly.

What's up with this? Is private void Dispose(bool) somehow getting called (or compiled to something that looks like protected virtual Dispose(bool)?

If this is some issue with Code Analysis and is the incorrect pattern, are there ways to detect this? Possibly with StyleCop?

Edit: After consideration, is it that a base class can call base.Dispose() which will hit private void Dispose(bool)? Even if it isn't able to pass in an argument?

Edit: Sample

public class A : IDisposable
{
    ~A()
    {
        this.Dispose(false);
    }

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

    private void Dispose(bool disposing) // Should be protected virtual void Dispose(bool)
    {
        Console.WriteLine("A");
    }
}

public class B : A
{
    protected virtual void Dispose(bool disposing) // Proper pattern.
    {
        Console.WriteLine("B");
    }
}

public static class Program
{
    static void Main(string[] args)
    {
        A a = new A();
        a.Dispose(); // Prints "A"

        B b = new B();
        b.Dispose(); // Prints "A"!
    }
}

As you can see from this, it makes using the dispose pattern totally unwieldy.

You can get around this a bit by hiding the public void Dispose(void) and then calling base.Dispose() somewhere. This then works 'similar' to the proper dispose pattern when calling B b = new B(); b.dispose(); except when calling A b = new B(); b.Dispose();, which only calls A's Dispose method.

public class B : A
{
    public void Dispose() // Causes CA error with or without "new".
    {
        this.Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing) // Proper pattern.
    {
        base.Dispose(); // Writes "A" (without quotes).
        Console.WriteLine("B");
    }
}

Basically, this whole thing seems terrible. Do we know if it is a bug that CA accepts private void Dispose(bool) and is there a way to at least throw a warning with StyleCop?

Edit: I don't think I should accept Alexandre's answer, as relative to the question I have it basically boils down to "Could be a bug", along with something that should be a comment. If anyone else has something more conclusive, I think that would be a more appropriate answer.

Nate Diamond
  • 5,525
  • 2
  • 31
  • 57
  • What about sealed classes? Protected should be used for open classes, private for sealed. – Ron Beyer Aug 26 '15 at 23:52
  • It works for either is the issue. We found a couple of places where unsealed classes were not throwing the issue. That makes sense though, that private is required for sealed classes. Still though, it would be nice for CA to throw if the class is unsealed (and it should know that as well). I think my edit shows why it may not be completely out of the woods for it to not throw, but it's still a little annoying. – Nate Diamond Aug 26 '15 at 23:54
  • Calling `base.Dispose()` would be a clear violation of the pattern, so I don't think that it explains it. I agree with your original thought that Code Analysis should report the case you presented. – sstan Aug 27 '15 at 00:28
  • I would use [another pattern](http://stackoverflow.com/a/2163702/31158) altogether. – Jordão Aug 27 '15 at 00:37
  • There are a lot of questions in this post. Which is the essential question: why the change in recommended pattern, or why does code analysis not complain? – Keith Payne Aug 27 '15 at 00:42
  • To me it's "Why does code analysis not complain? Is there a way to make something (code analysis or Style Cop) complain? – Nate Diamond Aug 27 '15 at 17:20
  • @Jordão that pattern is neat, but difficult to add to an already complex inheritance chain, very different from IDisposable which I can add to whatever part of the chain I want. Plus, that chain will likely violate Code Analysis, requiring a bunch of suppressions all over the place. Further, if the release resources are private we're in the same boat. I do agree that it is more clear, but it doesn't really have anything to do with what is being asked. – Nate Diamond Aug 27 '15 at 17:26
  • @Nate: Indeed it doesn't, that's why I added it as a comment. – Jordão Aug 27 '15 at 20:20
  • @Jordão Fair enough! After rereading what I wrote, I think I sounded harsher than I meant it. And still a good resource for people reading up on this in the future. – Nate Diamond Aug 27 '15 at 20:27

1 Answers1

6

Implementing a Dispose Method

The IDisposable interface requires the implementation of a single parameterless method, Dispose. However, the dispose pattern requires two Dispose methods to be implemented:

  • A public non-virtual (NonInheritable in Visual Basic) IDisposable.Dispose implementation that has no parameters.
  • A protected virtual (Overridable in Visual Basic) Dispose method.

Because the public, non-virtual (NonInheritable in Visual Basic), parameterless Dispose method is called by a consumer of the type, its purpose is to free unmanaged resources and to indicate that the finalizer, if one is present, doesn't have to run. Because of this, it has a standard implementation:

public void Dispose()
{
   // Dispose of unmanaged resources.
   Dispose (true);
   // Suppress finalization.
   GC.SuppressFinalize (this);
}

In the second overload, the disposing parameter is a Boolean that indicates whether the method call comes from a Dispose method (its value is true) or from a finalizer (its value is false).

When the garbage collector decides that your object is no longer needed, it'll try to finalize it in case you forgot to call the parameterless dispose method, because if you did and you are following the pattern, the call would be suppressed.

See: How Finalization Works

Private vs Protected Virtual:

You should always use protected virtual as the documentation says if you ever want to support subclasses that follows the pattern correctly.

Why some people use the private version? Maybe because inheritance was never their intention, specially if you are just generating methods on the fly using tools like Resharper, most of the time these methods are going to be private.

Why Code Analysis doesn't report the problem?

Could be a bug. Provide a small sample that gives the problem so that other people could test on their machines.

Alexandre Borela
  • 1,576
  • 13
  • 21
  • What different Dispose(bool) should do when the bool is true and false? – AksharRoop Mar 11 '16 at 14:53
  • At the end of the page(https://msdn.microsoft.com/en-us/library/system.object.finalize.aspx) you'll see an example, If true, you must dispose all resources, because you called the dispose method and want all the used memory back. If false, it means that the garbage collector is deleting the object, then you only need to dispose the unmanaged resources(objects that are created outside of your C# application), the rest of the objects can be left untouched as the garbage collector will delete them when it needs. – Alexandre Borela Mar 11 '16 at 15:20