38

I'm new to C#, so apologies if this is an obvious question.

In the MSDN Dispose example, the Dispose method they define is non-virtual. Why is that? It seems odd to me - I'd expect that a child class of an IDisposable that had its own non-managed resources would just override Dispose and call base.Dispose() at the bottom of their own method.

Thanks!

Sbodd
  • 11,279
  • 6
  • 41
  • 42
  • possible duplicate of [C# Finalize/Dispose pattern](http://stackoverflow.com/questions/898828/c-finalize-dispose-pattern) – Dour High Arch Sep 01 '10 at 18:03
  • 1
    @Dour: Actually, that question (and its answers) do not address the issue of a method in an `interface` missing the `virtual` keyword. – Robert Harvey Sep 01 '10 at 18:05
  • @Robert, it's not that the method in the interface is *missing* the virtual keyword. The *overloaded* Dispose(bool) is missing, and that's the one that needs the virtual keyword. On the other hand, putting a method signature in an interface pretty much forces the method to be public (or so it seems when I've tried to run counter to that), and the overload Dispose(bool) is protected in the full Finalize/Dispose pattern. – Cylon Cat Sep 03 '10 at 00:05

9 Answers9

19

Typical usage is that Dispose() is overloaded, with a public, non-virtual Dispose() method, and a virtual, protected Dispose(bool). The public Dispose() method calls Dispose(true), and subclasses can use this protected virtual method to free up their own resorces, and call base.Dispose(true) for parent classes.

If the class owning the public Dispose() method also implements a finalizer, then the finalizer calls Dispose(false), indicating that the protected Dispose(bool) method was called during garbage collection.

If there is a finalizer, then the public Dispose() method is also responsible for calling GC.SuppressFinalize() to make sure that the finalizer is no longer active, and will never be called. This allows the garbage collector to treat the class normally. Classes with active finalizers generally get collected only as a last resort, after gen0, gen1, and gen2 cleanup.

Cylon Cat
  • 7,111
  • 2
  • 25
  • 33
  • 6
    Your description is very precise. One remark though, the `public Dispose()` method should __always__ call `GC.SuppressFinalize()` when that declaring type is __not__ sealed. – Steven Sep 01 '10 at 15:20
  • 1
    @Steven, I'm not sure what "sealed" has to do with things, but as a broad pattern, any class that implements a finalizer also needs to implement IDisposable, so the finalizer and Dispose() implementation will be in the same class. Classes that inherit from classes implementing Dispose() should use the virtual protected Dispose(bool). If for some reason a class subclasses a Disposable class, and needs its own finalizer, then I would say that it's time to re-think the design. – Cylon Cat Sep 01 '10 at 17:08
  • 3
    @Cylon: [Part 1/3] I agree that any class with a finalizer should have a `Dispose` method. However, this doesn’t mean that a finalizer and `Dispose` should be together in the same class. This does not hold, because you not always design both classes (think about framework designers). Look for instance at `System.IO.Stream`. It implements `IDisposable`, but doesn’t have a finalizer. `FileStream` however, actually does implement a finalizer. Stream calls `SuppressFinalize`, even while it doesn’t implement a finalizer itself. – Steven Sep 01 '10 at 19:07
  • 3
    @Cylon: [Part 2/3] When `Stream` didn’t, `FileStream` had to call `SuppressFinalize` in it’s `Dispose(bool)` method, which isn’t that bad, but this doesn’t follow the ‘dispose pattern’. What would be worse is implementing an empty finalizer on the `Stream` class, because this would increase the change of objects keeping on the heap when developers forget to dispose their objects.Talking about bad design: `System.ComponentModel.Component` actually has an empty finalizer, and this causes all sorts of trouble when instances are not disposed properly (I’ve seen OOM being thrown because of this). – Steven Sep 01 '10 at 19:08
  • 4
    @Cylon: [Part 3/3] Last note, I noted “sealed”, because only when your class is sealed, you can be sure that nobody inherits from your class and thus add a finalizer. I hope this clears things up. – Steven Sep 01 '10 at 19:09
  • @Steven, I see your point about the Stream hierarchy. For the subclass, you'd want to suppress the finalizer only when Dispose(true); if it's Dispose(false), then you're already in the garbage collector and it's too late. As for discussion of when to use "sealed" or not, that's a separate discussion, but I wouldn't make a class sealed just to prevent a finalizer from being attached in a subclass. – Cylon Cat Sep 01 '10 at 19:35
  • 1
    @Cylon: I agree that you shouldn't seal a class just to not have to call `SuppressFinalize`. I think it is the other way around: You designed a class and made it sealed for a special reason (security for instance) and because it is sealed you don't have to call `SuppressFinalize`. – Steven Sep 02 '10 at 05:24
  • 3
    @Steven: Worth noting that, among the framework designers, sealing classes is the rule rather than the exception, as it can be difficult to evalue ahead of time all the different ways a user might make use of (and possibly break) your class in a derived class. Ergo, you don't need a special reason to seal a class. See http://blogs.msdn.com/b/ericlippert/archive/2004/01/22/61803.aspx – Robert Harvey Sep 02 '10 at 22:02
  • @Robert: This is very interesting. Also notice that Lippert's advice contradicts that of the Framework Design Guidelines who explicitly state: "DO NOT seal classes without having a good reason to do so." [2nd edition, page 209]. – Steven Sep 02 '10 at 22:42
  • @Steven, that may depend on what the definition of "a good reason" is. For Microsoft, and considering the .NET framework, that might be a good reason to seal classes. On the other hand, I'm building a framework for use in the application I work on. If someone breaks a class, I can go talk to them. Microsoft doesn't have that option. – Cylon Cat Sep 03 '10 at 00:01
  • @Cylon & @Robert: I thought that Lippert had other design goals with his C# team than the other groups in the BCL (and perhaps they still have), but I did some statistic research and found out that about 58% of the (public leaf) types in the framework are inheritable (tested with all System* assemblies on .NET 3.5sp1). Here's how I found out: http://4kcan.com/s/MjM4. – Steven Sep 03 '10 at 07:30
  • @Cylon & @Robert: In my last comment I meant to say "58% of the types are _NOT_ inheritable", so the majority of the classes in the BCL is actually sealed. – Steven Sep 05 '10 at 11:09
13

This is certainly not an obvious one. This pattern was especially choosen because it works well in the following scenario's:

  • Classes that don't have a finalizer.
  • Classes that do have a finalizer.
  • Classes that can be inheritted from.

While a virtual Dispose() method will work in the scenario where classes don't need finalization, it doesn't work well in the scenario were you do need finalization, because those types often need two types of clean-up. Namely: managed cleanup and unmanaged cleanup. For this reason the Dispose(bool) method was introduced in the pattern. It prevents duplication of cleanup code (this point is missing from the other answers), because the Dispose() method will normally cleanup both managed and unmanaged resources, while the finalizer can only cleanup unmanaged resources.

Steven
  • 166,672
  • 24
  • 332
  • 435
  • 2
    I thought this was a more direct answer to the actual question that was asked, "Why should Dispose() be non-virtual?" A further clarification might be to explain that, regarding the "two types of clean-up", a future derived class might need finalization (and therefore its own `Dispose(bool)`) even if the base class didn't. If that happens and if the base class didn't provide a `virtual Dispose(bool)` then the derived class implementation can't call `base.Dispose(bool)` and so it's stuck not disposing the base properly. – bob Sep 12 '13 at 00:13
  • I don't think `Finalize` is the main issue. A bigger factor is to ensure that derived classes can add `Dispose` logic in the same way regardless of whether the basest-level class which implements `IDisposable` exposes a public `Dispose` method (vs. implementing `Dispose` explicitly). If `Foo1` implements `Dispose` explicitly, and `Foo2` inherits `Foo1` but also has a public `Dispose` method, what method should be overridden by `Foo3`, which inherits `Foo2`? Saying that all functionality should be in the protected virtual method, and all public methods should chain to that... – supercat Dec 08 '13 at 02:21
  • ...eliminates the ambiguity. – supercat Dec 08 '13 at 02:22
5

Although methods in an interface are not "virtual" in the usual sense, they can nevertheless still be implemented in classes that inherit them. This is apparently a convenience built into the C# language, allowing the creation of interface methods without requiring the virtual keyword, and implementing methods without requiring the override keyword.

Consequently, although the IDisposable interface contains a Dispose() method, it does not have the virtual keyword in front of it, nor do you have to use the override keyword in the inheriting class to implement it.

The usual Dispose pattern is to implement Dispose in your own class, and then call Dispose in the base class so that it can release the resources it owns, and so on.

A type's Dispose method should release all the resources that it owns. It should also release all resources owned by its base types by calling its parent type's Dispose method. The parent type's Dispose method should release all resources that it owns and in turn call its parent type's Dispose method, propagating this pattern through the hierarchy of base types.

http://msdn.microsoft.com/en-us/library/fs2xkftw.aspx

Robert Harvey
  • 178,213
  • 47
  • 333
  • 501
4

The Dispose method should not be virtual because it's not an extension point for the pattern to implement disposable. That means that the base disposable class in a hierarchy will create the top-level policy (the algorithm) for dispose and will delegate the details to the other method (Dispose(bool)). This top-level policy is stable and should not be overridden by child classes. If you allow child classes to override it, they might not call all the necessary pieces of the algorithm, which might leave the object in an inconsistent state.

This is akin to the template method pattern, in which a high-level method implements an algorithm skeleton and delegates the details to other overridable methods.

As a side note, I prefer another high-level policy for this particular pattern (which still uses a non-virtual Dispose).

Jordão
  • 55,340
  • 13
  • 112
  • 144
3

Calls through an interface are always virtual, regardless of whether a "normal" call would be direct or virtual. If the method that actually does the work of disposing isn't virtual except when called via the interface, then any time the class wants to dispose itself it will have to make sure to cast its self-reference to iDisposable and call that.

In the template code, the non-virtual Dispose function is expected to always be the same in the parent and the child [simply calling Dispose(True)], so there's never any need to override it. All the work is done in the virtual Dispose(Boolean).

Frankly, I think using the Dispose pattern is a little bit silly in cases where there's no reason to expect descendant classes to directly hold unmanaged resources. In the early days of .net it was often necessary for classes to directly hold unmanaged resources, but today in most situations I see zero loss from simply implementing Dispose() directly. If a future descendant class needs to use unmanaged resources, it can and typically should wrap those resources in their own Finalizable objects.

On the other hand, for certain kinds of method there can be advantages to having a non-virtual base class method whose job is to chain to a protected virtual method, and having the virtual method be called Dispose(bool) is really no worse than VirtDispose() even if the supplied argument is rather useless. In some situations, for example, it may be necessary for all operations on an object to be guarded by a lock which is owned by the base-class object. Having the non-virtual base-class Dispose acquire the lock before calling the virtual method will free all the base classes from having to worry about the lock themselves.

supercat
  • 77,689
  • 9
  • 166
  • 211
2

The reason the sample's Dispose() method is non-virtual is because they take over the entire process in that example, and leave subclasses with the virtual Dispose(bool disposing) method to override. You'll notice that in the example, it stores a boolean field to ensure that the Dispose logic does not get invoked twice (potentially once from IDisposable, and once from the destructor). Subclasses who override the provided virtual method do not have to worry about this nuance. This is why the main Dispose method in the example is non-virtual.

Kirk Woll
  • 76,112
  • 22
  • 180
  • 195
  • Example would be more obvious if it also had a finalizer calling Dispose(bool). – Joel Rondeau Sep 01 '10 at 15:15
  • Too bad the Disposed field is used in a way that is useless to derived classes. Better would have been an Integer DisposalState field which could be tested and set using Interlocked.Exchange in the non-virtual Dispose method. – supercat Feb 17 '11 at 18:16
1

I've got a quite detailed explanation of the dispose pattern here. Essentially, you provide a protected method to override that is more robust for unmanaged resources instead.

Community
  • 1
  • 1
thecoop
  • 45,220
  • 19
  • 132
  • 189
0

If the base class has resources that need to be cleaned up at Dispose() time, then having a virtual Dispose method that's overridden by an inheriting class prevents those resources from being released unless the inheriting class specifically calls the base's Dispose method. A better way would implement it would be to have each derived class implement IDisposable.

SwDevMan81
  • 48,814
  • 22
  • 151
  • 184
  • Well...if you're overriding virtual methods and not calling the base implementation, you're already in for a world of hurt. – Kirk Woll Sep 01 '10 at 15:10
  • That is true, which is why I suggested implementing `IDisposable` for each derived class. Making `Dispose` virtual is possible, but the derived classes must call the base class `Dispose` method – SwDevMan81 Sep 01 '10 at 15:11
0

Another, not so obvious reason is to avoid the need to suppress CA1816 warnings for derived classes. These warnings look like this

[CA1816] Change Dispose() to call GC.SuppressFinalize(object). This will prevent derived types that introduce a finalizer from needing to re-implement 'IDisposable' to call it.

Here is an example

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

        GC.SuppressFinalize(this);
    } 
}

public class Derived : Base 
{
    public override void Dispose() // <- still warns for CA1816
    {
        base.Dispose();

        ...
    }
}

You can resolve this by just adopting the recommended Dispose pattern.

ViRuSTriNiTy
  • 5,017
  • 2
  • 32
  • 58