Correct scope of using using

Question

I'm in a quarrel with a colleague regarding the appropriate scope of the using statement. This is the method in question.

public Guid IsServerReachable()
{
  try
  {
    WhoAmIResponse whoAmI;
    using (OrganizationServiceProxy service = GetService())
      whoAmI = service.Execute(new WhoAmIRequest()) as WhoAmIResponse;
    return whoAmI.UserId;
  }
  catch { return Guid.Empty; }
}

One of us claims that the using statement should include the declaration of whyAmI, while the other argues that it's only the service instance that needs to be using-ified. I'm not telling which is my theory but one is clearly wrong. Which?

Answer 1

using (OrganizationServiceProxy service = GetService())
    whoAmI = service.Execute(new WhoAmIRequest()) as WhoAmIResponse;

Will be equivalent to:

OrganizationServiceProxy service = GetService();
try
{
    whoAmI = service.Execute(new WhoAmIRequest()) as WhoAmIResponse;
}
finally
{
    if (myRes!= null)
        // Call the object's Dispose method.
        ((IDisposable)service).Dispose();
}

Answer 2

Both is correct. I tend to write this:

public Guid IsServerReachable()
{
    try
    {
        using (OrganizationServiceProxy service = GetService())
        {
            WhoAmIResponse whoAmI = service.Execute(new WhoAmIRequest()) as WhoAmIResponse;
            return whoAmI.UserId;
        }
    }
    catch { return Guid.Empty; }
}

This does not have any influence on whether whoAmI is disposed of - the only thing getting automatically disposed of is service.

If WhoAmIResponse was also an IDisposable, you'd have to write the following to automatically release both:

    using (OrganizationServiceProxy service = GetService())
    using (WhoAmIResponse whoAmI = service.Execute(new WhoAmIRequest()) as WhoAmIResponse)
        return whoAmI.UserId;

Answer 3

Since the declaration of whoAmI doesn't really make any difference anything in terms of performance/scoping in this case. All it really comes down to is should the property access of whoAmI.UserId also be included in the using. From an IL perspective, the only functional difference between the two is the order in which the OrganizationalServiceProxy.Dispose method is invoked and when the WhoAmIResponse.UserId is accessed.

(EDIT: I don't think there's any real issue with how the try/catch is handled to return a default value, and that doesn't seem to be part of the question, so this is also omitted)

In your posted code (simplified to make the IL clearer):

public Guid IsServerReachableOutsideUsingScope()
{
    WhoAmIResponse whoAmI;
    using(var service = new Service())
        whoAmI = service.Execute();
    return whoAmI.UserId;
}

Results in IL of:

IL_0000:  newobj      UserQuery+Service..ctor
IL_0005:  stloc.1     // service
IL_0006:  ldloc.1     // service
IL_0007:  callvirt    UserQuery+Service.Execute
IL_000C:  stloc.0     // whoAmI
IL_000D:  leave.s     IL_0019
IL_000F:  ldloc.1     // service
IL_0010:  brfalse.s   IL_0018
IL_0012:  ldloc.1     // service
IL_0013:  callvirt    System.IDisposable.Dispose
IL_0018:  endfinally  
IL_0019:  ldloc.0     // whoAmI
IL_001A:  callvirt    UserQuery+WhoAmIResponse.get_UserId
IL_001F:  ret         

Whereas declaring everything within the using block:

public Guid IsServerReachableWithinUsingScope()
{
    using(var service = new Service())
    {
        WhoAmIResponse whoAmI = service.Execute();
        return whoAmI.UserId;
    }
}

Produces IL:

IL_0000:  newobj      UserQuery+Service..ctor
IL_0005:  stloc.0     // service
IL_0006:  ldloc.0     // service
IL_0007:  callvirt    UserQuery+Service.Execute
IL_000C:  stloc.1     // whoAmI
IL_000D:  ldloc.1     // whoAmI
IL_000E:  callvirt    UserQuery+WhoAmIResponse.get_UserId
IL_0013:  stloc.2     // CS$1$0000
IL_0014:  leave.s     IL_0020
IL_0016:  ldloc.0     // service
IL_0017:  brfalse.s   IL_001F
IL_0019:  ldloc.0     // service
IL_001A:  callvirt    System.IDisposable.Dispose
IL_001F:  endfinally  
IL_0020:  ldloc.2     // CS$1$0000
IL_0021:  ret         

If it matters that your service not be disposed of before accessing the property (say in the context of NHibernate lazily loaded collections) then the order definitely matters. If it doesn't matter, then the biggest question should be what you and your team care about most. If you don't mind mixing and matching using calls so some have braces and some don't, then go ahead with what you have.

Possibly what might be something to consider is order of exception handling if accessing WhoAmIResponse.UserId has side effects. If the Dispose call on your service throws an exception, in your original code (IsServerReachableOutsideUsingScope) it will never have accessed your property and thus never executed its side effects. In the second block of code (IsServerReachableWithinUsingScope), it will have accessed and executed side effects of using the UserId property, then run the Dispose which throws an exception.

These are fairly rare cases (EDIT: and it should be noted that both get-access side effects and Dispose() throwing exceptions are considered bad practices), and I would suggest if it is the case here, then you should consider these for correctness. If these are non-issues (no side effects and do not care about order of access/disposal) then use what you and your team feel is more maintainable/readable in the long run.

Answer 4

The using statement must include the declaration of the object that is supposed to be disposed when the statement terminates. Your code is correct as long as OrganizationServiceProxy implements IDisposable and WhoAmIResponse does not.

In case of doubt, it is normally useful to rewrite the using block as a try-finally block:

OrganizationServiceProxy service = GetService();
try {
   whoAmI = service.Execute(new WhoAmIRequest()) as WhoAmIResponse;
} finally {
    service.Dispose();
}

Answer 5

It is best practice for the scope of any using statement to be as small as practical. So long as the objects returned by your OrganizationServiceProxy aren't disposed of when it runs the Dispose method, the indicated scope is perfectly acceptable.