Is it OK doing a return from inside using block

Question

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 line 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.

Answer 1

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.)

Answer 2

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.

Answer 3

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.

Answer 4

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 returning.

Answer 5

This warning is probably related to the principle of "single exit point". It is discussed here: http://c2.com/cgi/wiki?SingleFunctionExitPoint