CriticalError, Certainty 95, for TypesThatOwnDisposableFieldsShouldBeDisposable
Fxcop says that my class (PrintText) which instantiates an IDisposable type (Font) should also implement IDisposable. The documentation goes on to say that I should not exclude a message from this rule.
My PrintText class is just a way to encapsulate a particular usage of PrintDocument. I need a font for what I am doing. I instantiate it in PrintText.Print, and I dispose it in the PrintDocument.PrintPage event handler. My font object is a property of the class only because two methods in the class use it.
So, Fxcop is complaining (sternly) about something that I have been pushed into by MS's event driven printing design. I am taking care of resource business correctly, and fxcop doesn't (probably can't) see that. In other settings, within a single method, I instantiate, use, and dispose various IDisposable objects, and Fxcop is clueless as to the correctness of my resource handling. Because PrintDocument printing is event driven, I am coerced into a class level (vice local) font field. If printing were procedural rather than event driven, my font would be a local in a method, and Fxcop would be none the wiser.
To summarize, the presence of a disposable type in a class trips this fxcop rule, and that says little about the correctness of code regarding freeing resources. Maybe this rule should be a warning, or perhaps it should apply only to public IDisposable fields.
Types with disposable fields should be disposable
dasmanojk
There is also a third pattern that Nick Guerrera suggested (this will only work in C# 2.0):
public class PrintClass
{
public void Print()
{
using (Font font = new Font("Tahoma", 8.25F))
{
using (PrintDocument document = new PrintDocument())
{
document.PrintPage += delegate(object sender, PrintPageEventArgs e)
{
e.Graphics.DrawString("Hello World!", font, SystemBrushes.Control, 0, 0);
};
document.Print();
}
}
}
}
Basically you want to minimize the exposure of a field or variable. If PrintPage is the only method that is going to use the Font, then you want reduce the amount of code (ie methods) that have access to it.
1. As FxCop does not actually run the code it is analyzing, there are certain code patterns that FxCop will incorrectly fire on, so there are times where exclusion as a false positive is the only thing you can do.
2. Have you measured how expensive creating a Font is Compared to sending output to a physical printer (or any general I/O), I think you will find that it is minimal in comparision.
3. It turns out that if you assign the created Font to a local variable first and then assign it to a member field, then FxCop will not fire on the field. In C# the code pattern for this actually causes the Font stored in a temporary local variable and then assigned to _Font. Hence, why this didn't fire for me.
There are no plans to change the way this pattern fires. If anything, this rule should consider not firing on public fields, as we don't know what other classes may have accessed the field and are using it.
MarcCBrooks
re 1, sure, but that is giving up on the issue.
re 2, that should work, and the only cost is instantiating a font once per page.
re 3, that doesn't work (I tried it) - the code works but fxcop still complains about my class not implementing IDisposable.
The more I think about this issue, the more I think the fxcop rule should apply to public IDisposable fields and not to private ones, or perhaps apply only as a warning to private fields.
Neelesh Thakur
FxCop is unable to determine that your PrintPage eventhandler will be called when you call PrintDocument.Print and therefore ultimately dispose of the Font.
You can work around this by doing a couple of things:
1. Exclude this violation via in-source suppression or in the FxCop project itself
2. Only instantiate the font in the PrintPage handler:
public class PrintClass
{
public void Print()
{
using (PrintDocument document = new PrintDocument())
{
document.PrintPage += new PrintPageEventHandler(OnDocumentPrintPage);
document.Print();
}
}
void OnDocumentPrintPage(object sender, PrintPageEventArgs e)
{
using (Font font = new Font("Tahoma", 8.25F))
{
e.Graphics.DrawString("Hello World!", font, SystemBrushes.Control, 0, 0);
}
}
}
3. Dispose of the font in the Print method, rather than the PrintPage method:
public class PrintClass
{
private Font _Font;
public void Print()
{
using (_Font = new Font("Tahoma", 8.25F))
{
using (PrintDocument document = new PrintDocument())
{
document.PrintPage += new PrintPageEventHandler(OnDocumentPrintPage);
document.Print();
}
}
}
void OnDocumentPrintPage(object sender, PrintPageEventArgs e)
{
e.Graphics.DrawString("Hello World!", _Font, SystemBrushes.Control, 0, 0);
}
}
As far as maintenance goes, I feel it is better to have the same method that creates the object, to also dispose of it.