Hi Jesper,

I think this code sample needs some changes. Microsoft has a template, but you have to consider some topics:

- virtual (polymorphistic) mehods for releasing managed and unmanaged code should be called Dispose(bool disposed)
- keep releasing methods for unmanaged and managed code(like "FreeManagedResources" and "FreeUnmanagedResources") always private for the class itself, it is the responsibility of the class and not for derived classes
- a finalizer is expensive to introduce. You only need them when the class holds unmanaged resources to be sure that unmanaged resources are always cleaned up by the GC.
- [MethodImpl(MethodImplOptions.Synchronized)] is not needed by default.
- use camelCasing in stead of m_ for class fields (this is Microsoft's default), but that's just code style.

Hi Patrick,

Thank you for comment. Nice to get feedback :O)

First look at page 98 of Juval Lowys book "Programming .NET components." I guess I am a fan of his. :O) The Dispose pattern presented by me is just an improved version of his Dispose Pattern (and nicer looking :O) ). He makes what he almost calls a "bulletproof" Dipose pattern. This of course also means that not all parts of it is necessary all the time. But it certainly helps.

I will answer each of your comments here:

"virtual (polymorphistic) mehods for releasing managed and unmanaged code should be called Dispose(bool disposed)"

- I am not sure were you get this "rule" from ? Are you just referering to the naming of FreeManagedRessources and FreeUnmanagedRessources ? I guess DisposeManagedResources could be a better name.

"keep releasing methods for unmanaged and managed code(like "FreeManagedResources" and "FreeUnmanagedResources") always private for the class itself, it is the responsibility of the class and not for derived classes"

- Hmm... This sort of implies that you never inherit a Disposable class. If you call Dispose on the derived class you would have to override the Dispose method itself repeating the whole pattern.

"a finalizer is expensive to introduce. You only need them when the class holds unmanaged resources to be sure that unmanaged resources are always cleaned up by the GC."

- A finalizer should ONLY be used for releasing unmanaged resources (as you say yourself), and that is exactly what I do. Look at the code again :O) Not only that. A finalizer should ALWAYS be used for releasing unmanaged resources. because otherwise you have no guarantee they will be released. Of course you have a point that if there are no unmanaged resources there are no reason to provide a finalizer. But then again, you will not be able to convince me that a finalizer that does nothing is expensive :O) And furthermore this pattern is just a best practice attempt, and consequently I have to provide the finalizer.

"[MethodImpl(MethodImplOptions.Synchronized)] is not needed by default."

- I guess you are right. But again this is a best practice attempt. This attribute means exactly the same as lock(this), but is better because it locks the entire method which lock(this) only does if you do it right ;O). Just consider two different threads trying to Dispose the same object, without the lock they could both be trying to free the same unmanaged ressources which could potentially be dangerous. BTW: The lock statement I also got from Juval's book. I just brought it into the present with the MethodImpl attribute, which Juval often prefers too.

"use camelCasing instead of m_ for class fields (this is Microsoft's default), but that's just code style."

- Well it is just coding style, but I strongly disagree :O) I code by IDesigns coding standard which uses m_ and which is written by - guess who - Juval Lowy :O) And why is it wrong to use camelCasing ? Because it forces you to use the this keyword, which should only be used during development IMHO (and of course for extension methods and constructors. Microsoft is split on this matter. Their generated code often looks like what you describe, and at other times uses an underscore as prefix. The first one is wrong (IMHO), the second is a matter of coding style.

Again thank you for the feedback. I didn't understand your first point, but the rest made me think, even though I disagree. I haven't been changing this pattern for a few years now and I think I will keep on using it. At least it has not given me any trouble. :O)

Hi Jesper,

The template I am using is coming from Microsoft's MSDN site and is also used by Bill Wagners "Effective C#: 50 Specific Ways to Improve Your C#". IDesign's coding standards are very useful and helpful but are not always in line with Microsoft's guidelines. This also the fault of Microsof itself where there some gaps in the guidelines. The one and only coding guidelines reference that is used by all MS teams is the book "Framework Design Guidelines" and the Dispose template I am referring to is coming from that book also. MSDN is also referering to that book but can only state slices of the text from it due to legal/copyright stuff.

Here are my replies again, read the text after ->>>>>

- I am not sure were you get this "rule" from ? Are you just referering to the naming of FreeManagedRessources and FreeUnmanagedRessources ? I guess DisposeManagedResources could be a better name. ->>>> this rule is coming from the Dispose template in MSDN and in Framework Design Guidelines. All MS teams are implementing this way, and also check the .NET library itself how disposable components are implemented (use reflector for it for example). When you design a disposable component (class) that is not sealed, you should declare a virtual method named "Dispose(bool disposing)". When you derive from that component, you can override it, dispose your managed and unmanaged code and then call base.Dispose(disposing). You can reuse your methods called FreeManagedRessources and FreeUnmanagedRessources in this overriden dispose method of course ;-)

This is the Dispose template I am using (and is coming from MS/Framework Design Guidelines)


public class MyHelper : IDisposable
{
private bool disposed;

public MyHelper()
{
}

~MyHelper()
{
Dispose(false);
}

#region IDisposable Members

public void Dispose()
{
Dispose(true);

GC.SuppressFinalize(this);
}

#endregion

protected virtual void Dispose(bool isDisposing)
{
if (!this.disposed)
{
if (isDisposing)
{
//cleanup managed resources here, you may also chain it to a private method in this class
}

//cleanup unmanaged resources, you may chain it to a private method in this class

this.disposed = true;
}
}
}
}

and now a derived class:




public class MyDerivedHelper : MyHelper
{
private bool disposed;

protected override void Dispose(bool isDisposing)
{
if (!this.disposed)
{
if (isDisposing)
{
//cleanup managed resources here, you may also chain it to a private method in this class
}

//cleanup unmanaged resources, you may chain it to a private method in this class

this.disposed = true;
}

//call the base class's dispose
base.Dispose(isDisposing);
}
}


- Hmm... This sort of implies that you never inherit a Disposable class. If you call Dispose on the derived class you would have to override the Dispose method itself repeating the whole pattern. ->>>>> see me previous comment, you can call those methods within the virtual or overriden Dispose method.
- A finalizer should ONLY be used for releasing unmanaged resources (as you say yourself), and that is exactly what I do. Look at the code again :O) Not only that. A finalizer should ALWAYS be used for releasing unmanaged resources. because otherwise you have no guarantee they will be released. Of course you have a point that if there are no unmanaged resources there are no reason to provide a finalizer. But then again, you will not be able to convince me that a finalizer that does nothing is expensive :O) And furthermore this pattern is just a best practice attempt, and consequently I have to provide the finalizer. ->>>>> If an object uses a finalizer and it is being collected by the GC when it is out of scope, the GC places it on a finalizer queue. In the next GC cycle all finalizers on the queue are run (on a single thread in the current implementation) and the memory from the finalized objects reclaimed. It's fairly obvious from this why you don't want to do clean up in a finalizer: it takes two GC cycles to collect the object instead of one and there is a single thread where all finalizers are run while every other thread is suspended, so it's going to hurt performance.

- I guess you are right. But again this is a best practice attempt. This attribute means exactly the same as lock(this), but is better because it locks the entire method which lock(this) only does if you do it right ;O). Just consider two different threads trying to Dispose the same object, without the lock they could both be trying to free the same unmanaged ressources which could potentially be dangerous. BTW: The lock statement I also got from Juval's book. I just brought it into the present with the MethodImpl attribute, which Juval often prefers too. ->>>>> it may be a best practice when multi threading is used in a project, but that is not always the case...

- Well it is just coding style, but I strongly disagree :O) I code by IDesigns coding standard which uses m_ and which is written by - guess who - Juval Lowy :O) And why is it wrong to use camelCasing ? Because it forces you to use the this keyword, which should only be used during development IMHO (and of course for extension methods and constructors. Microsoft is split on this matter. Their generated code often looks like what you describe, and at other times uses an underscore as prefix. The first one is wrong (IMHO), the second is a matter of coding style. ->>>>> well, I think this could lead to a religious discussion, but I only want to say this: MS .NET library had different teams, where the guidelines stated in the Framework Design Guidelines(FDG) book was not landed on every team, also enforcing the rules was not always the case, and there was a time-delivering pressure at the different teams. That leads to inconsistent usage of private members (and variables). Nowdays MS is enforcing the guidelines from the FDG strongly, but I think "old" code could still be in its old inconsistent state when analyzing it with Reflector for example. But... when using the MS guidelines stated in FDG, you adapt a style that is becoming more and more common practice and easier for teams to add new teammembers that are familiar with these guidelines.



Patrick

Well A short followup. Bill Wagners book also influenced my Dispose Pattern. I own it and read it with joy. The inheritance principle is the same I just make it explicit where to free managed and unmanaged code, which I think is better.

In my pattern I try to make the best pattern possible for all cases. I could make 10 patterns for 10 different cases instead, but that was not the point. You are right about not everything being multithreaded, well then just remove the lock in those cases :O) But it could become multithreaded in the future.
It would be the same as not using Transactions if you have only Database reads. You never know what the future will bring.

You are probably right about the Finalizer performance thing. But still the alternative is that your unmanaged resources have a chance of not being cleaned up. If you are correct and it has a serious impact, the best choice would be to remove the destructor when having no unmanaged resources. Would be nice to test this some day.

Although I had similar thoughts in templatizing out the Dispose(bool) method, the problem is that implementing it like this enforces that managed resources from the entire class hierarchy should *always* be released in their entirety before *any* unmanaged resources anywhere along the hierarchy chain. Although somewhat rare, this may not be the case for derived classes. I only bring this up because you're trying for a "best practice" approach...

As for the locking issue, this should really be done only on classes that are synchronized between threads. The overhead of locking is somewhat small, but can quickly add up when using a lot of small classes. This is more of a nit, but again, you are trying for a "best practice" type of implementation... ;-)

The other issue, of course, is that you should point out that subclasses which override the Free___Resources() methods should call base.Free___Resources() after doing their work but before returning, to make sure that base classes get a chance to clean their resources up too.

Hi Anonymous C :O)

I could consider switching the freeing of managed and unmanaged resources, but on the other hand the freeing of managed resources is really, about freeing unmanaged resources. So in the pattern I have described unmanaged resources will be freed in refered classes first, and in the "top" class last. If I switch the statements it will be the other way around. Will any of these be fool proof ? I guess I will have to give it some thought.

About the locking comment. One size fits all is not always good that is true. So you could remove the lock statement when you are sure it is not needed, or rather when performance demands it. I mean when can you really be sure that your class will never at some point in the future be part of a multithreaded scenario :O)

You are right I should have pointed out that one should call base.Free...() last in derived classes.
Denne kommentar er ikke blevet screened af en ekstern service.