Is it OK doing a return from inside using block
I am doing a code review, and have found alot of code with the following format:
public MyResponse MyMethod(string arg)
{
using (Tracer myTracer = new Tracer(Constants.TraceLog))
{
MyResponse abc = new MyResponse();
// Some code
return abc;
}
}
When I run a code analysis I get a CA2000 warning Microsoft.Reliability
Should the code be rewritten as:
public MyResponse MyMethod(string arg)
{
MyResponse abc = new MyResponse();
using (Tracer myTracer = new Tracer(Constants.TraceLog))
{
// Some code
}
return abc;
}
Or does it not matter?
Edit
The lin开发者_Python百科e on which it is reporting the warning is:
MyResponse abc = new MyResponse();
MyResponse is a standard Dataset.
The full error message is:
Warning 150 CA2000 : Microsoft.Reliability : In method 'xxxxx(Guid, Guid)', object 'MyResponse ' is not disposed along all exception paths. Call System.IDisposable.Dispose on object 'MyResponse ' before all references to it are out of scope.
No, it doesn't matter.
The finally
block that's implicitly generated by the using
statement to handle disposal will execute no matter where you put the return
.
Are you sure that the CA2000 relates to myTracer
and not abc
? I would guess that the warning is occurring because MyResponse
implements IDisposable
and you're not disposing abc
before returning. (Either way, your suggested rewrite should make no difference to the warning.)
Your rewrite will not fix that CA2000 warning, because the problem is not the Tracer
object, but the MyResponse
object.
The documentation states:
The following are some situations where the using statement is not enough to protect IDisposable objects and can cause CA2000 to occur.
Returning a disposable object requires that the object is constructed in a try/finally block outside a using block.
To fix the warning without messing with the stack trace of your exceptions (<- click, it's a link), use this code:
public MyResponse MyMethod(string arg)
{
MyResponse tmpResponse = null;
MyResponse response = null;
try
{
tmpResponse = new MyResponse();
using (Tracer myTracer = new Tracer(Constants.TraceLog))
{
// Some code
}
response = tmpResponse;
tmpResponse = null;
}
finally
{
if(tmpResponse != null)
tmpResponse .Dispose();
}
return response;
}
Why? Please see the example in the linked documentation.
The warning is probably about MyResponse
, which is IDisposable
.
Why does the warning appear?
If a MyResponse
object is constructed, but code later in the method causes an exception to be thrown, then all references to this object will be lost (we only had one, and didn't manage to return it). This means that Dispose
cannot be called on the object anymore, and we will be relying on the class finalizer to clean up any resources.
Does it matter?
Generally speaking, it will only matter if:
- The
IDisposable
encapsulates a resource that might be needed "soon" by other parts of the program or by another process - An exception is thrown before the method returns, to trigger the "problem"
- That resource is not released by the finalizer soon enough, or for some reason the finalizer never runs but your application doesn't go down
So no, it shouldn't really matter.
How to fix it?
public MyResponse MyMethod(string arg)
{
MyResponse abc = null;
try {
abc = new MyResponse();
using (Tracer myTracer = new Tracer(Constants.TraceLog))
{
// Some code
return abc;
}
}
catch {
if (abc != null) {
abc.Dispose();
}
throw;
}
}
This ensures that if control exits the method by means of an exception, abc
is either null
or has been properly disposed.
Update
It turns out when using this way of handling things, an exception explicitly thrown from inside MyMethod
will be rethrown and have the line number of the first stack frame mutated to point to the throw;
statement.
Practically this means that if you have multiple throw
statements inside MyResponse
, and they throw the same type of exception with the same message, you will not be able to tell which throw
was responsible exactly when you catch the exception.
This is IMHO a purely academic problem, but I mention it for completeness.
It doesn't really matter. But, contrary to @Aliostad, I think version 2, with the return
outside of the using
block is better style.
My rationale goes like this:
The using
block denotes something that is "opened" and "closed". This is a kind of cheep transaction. Closing the using
block says we have done our work and it is safe now to continue with other stuff, like return
ing.
This warning is probably related to the principle of "single exit point". It is discussed here: http://c2.com/cgi/wiki?SingleFunctionExitPoint
精彩评论