CODEWITHSUNDEEP
c#design-patternsidisposable
MikeS159
MikeS159

Reputation: 1974

Implementing IDisposable correctly on parent classes in C#

I have a class which implements the C# SerialPort which used to look like this:

public class AsyncSerial : IDisposable
{
    SerialPort newPort; //Parameters declared in my constructor
    //Constructor and other methods

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

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

This throws up no warnings in code analysis (I got the code from MSDN as an example of how to do it properly).

Since I was only ever going to declare one SerialPort I figured I would make my class a child of SerialPort, but now I get warnings that I can't seem to fix.

public class AsyncSerial : SerialPort
{
    //Constructor and other methods

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

    protected new virtual void Dispose(bool disposing)
    {
        if(disposing)
        {
            this.Close();
            this.Dispose();
        }
    }
}

Code warnings said the dispose methods should be new as they hide members, which I did, but I also get:

"Warning CA1063 Ensure that 'AsyncSerial.Dispose()' is declared as public and sealed"

Making it sealed means it has to be marked override (or I get compiler errors), making it override means it can be new, so I end up with:

Error CS0506 'AsyncSerial.Dispose()': cannot override inherited member 'Component.Dispose()' because it is not marked virtual, abstract, or override

I don't know the 'correct' way to implement disposing on a class with IDisposable in the parent class. Every example I find only fits having IDisposable as a base, but making my class 

public class AsyncSerial : SerialPort, IDisposable
{
    //code
}

gives me a code analysis warning because SerialPort already implements IDisposable.

Should I just suppress the warning about Ensuring that 'AsyncSerial.Dispose()' is declared as public and sealed, or is there a correct way to do this which doesn't give code analysis warning.

Upvotes: 3

Views: 1801

Answers (3)

supercat
supercat

Reputation: 81277

The Dispose Pattern was designed to allow derived types to add disposal logic in consistent fashion without regard for whether the parent type has a public Dispose method or implements IDisposable.Dispose explicitly. Types which derive from a type that follows the pattern should simply override Dispose(bool) regardless of how the parent class implements uses a public method or explicit implementation for IDisposable.Dispose().

Although the design of the Dispose pattern was based on the flawed assumption that publicly-exposed objects of inheritable types will often need to incorporate finalizers directly (rather than encapsulating unmanaged resources in privately-held instances of private types whose purpose is to clean up those resources), compiler-generated cleanup logic in C++/CLI (and maybe other languages as well) relies upon the pattern and so it's a good idea to use it with inheritable classes which may be used by other people.

Upvotes: 1

Matias Cicero
Matias Cicero

Reputation: 26321

You don't need to declare a public void Dispose() method on the child class, because it was already inherited from the base one (The compiler won't let you anyways, unless you hide the base implementation with the new keyword).

You also don't need to override the base class's protected virtual void Dispose(bool) if you're not going to dispose anything specific to this child class.

If you were to have an IDisposable reference in your child class, then you should override your base class's method:

public class AsyncSerial : SerialPort, IDisposable
{
    // SomeClass implements IDisposable
    private SomeClass _disposableInstance;

    // ...

    protected override void Dispose(bool disposing)
    {
        if(disposing)
        {
            if(_disposableInstance != null)
                _disposableInstance.Dispose();
        }

        // Call the base Dispose, to release resources on the base class.
        base.Dipose(disposing);
    }
}

Upvotes: 0

Jon Skeet
Jon Skeet

Reputation: 1503290

Your subclass should override Dispose(bool disposing) if anything - that's the whole point of having that method at all, really.

However, I suspect that the base class will make the right calls anyway, so you shouldn't need to do anything, unless you have extra resources to release which aren't released in Close(). If that's the case, do that in Dispose(bool disposing):

protected override void Dispose(bool disposing)
{
    // Allow the base class to release resources
    base.Dispose(disposing);
    // Release any extra resources here 
}

Note that your current implementation will lead to a StackOverflowException as your two Dispose overloads call each other.

Upvotes: 8

Related Questions