c# - is it ok to embed a "try/catch" within a "using" statement for a web request? Is my code correct?

Question

Hi

Is it ok to embed a "try/catch" within a "using" statement for a web request? Is my code correct? That is my requirements are:

1. Want to use the "using" statement to make sure resources are released in any case for HttpWebResponse

   • But still want to do some custom stuff if there is an exception re HttpWebResponse and "response = (HttpWebResponse)request.GetResponse();" in particular.

My source code:

        var result = new HttpHeaderInfo();
        HttpWebRequest request = null;
        HttpWebResponse response = null;
        using (response)
        {
            try
            {
                request = (HttpWebRequest)WebRequest.Create(uri);
                request.Method = "HEAD";
                request.KeepAlive = false;
                request.Timeout = Properties.Settings.Default.WebTimeoutDefault;

                response = (HttpWebResponse)request.GetResponse();
                result.LastModified = response.LastModified;
                result.ContentType = response.ContentType;
                result.StatusCode = response.StatusCode;
                result.ContentLength = response.ContentLength;
            }
            catch (Exception ex)
            {
                if (ex is InvalidOperationException ||
                    ex is ProtocolViolationException ||
                    ex is WebException)
                {
                    result.HttpError = ex;
                    result.LastModified = System.DateTime.MinValue;
                    result.ContentType = null;
                }
                else { throw; }
            }

        }

thanks

Answer 1

This is totally OK. You handle the exception and don't want it bubbling up further, that's just fine, and nested try/catch/finally blocks are no problem. (Internally a 'using' like this is just a try/finally.)

UPDATE: read a little closer, and I think you actually want the using inside the 'try' block - the line where you actually put an object in the 'response' variable is where you want the 'using' block to begin. Does it actually compile as-is?

Answer 2

It is OK, but a little redundant; in a general sense, you could easily remove the using block, add a finally block after the catch, and explicitly call Dispose in there, which would reduce the nesting in your code.

In a more specific sense, what bugs me a little is that you don't actually assign response until you get inside the using block, and the explicit variable declarations are unnecessary and confusing in this context. I would rewrite it as:

HttpHeaderInfo result;
try
{
    var request = (HttpWebRequest)WebRequest.Create(uri);
    request.Method = "HEAD";
    request.KeepAlive = false;
    request.Timeout = Properties.Settings.Default.WebTimeoutDefault;

    using (HttpWebResponse response = (HttpWebResponse)request.GetResponse())
    {
        result = new HttpHeaderInfo();
        result.LastModified = response.LastModified;
        result.ContentType = response.ContentType;
        result.StatusCode = response.StatusCode;
        result.ContentLength = response.ContentLength;
    }
}
catch (WebException ex)
{
    // etc.
}

This is a lot clearer than the original form. Also note that I'm catching WebException, not the generic System.Exception. You should catch specific exception types instead of catching generic exceptions and then checking their type.

Answer 3

Others have pointed this out as a potential problem, but I want to raise it as a very definite problem: your using statement is doing you no good at all at the moment.

When you write a using statement like this:

SomeType x = value1;
using (x)
{
    x = value2;
}

it is value1 which will be disposed at the end of the block, not value2. In your code, response is null until inside the block; the WebResponse you end up with will not be disposed.

You should be seeing a warning about this, along these lines:

warning CS0728: Possibly incorrect assignment to local 'response' which is the argument to a using or lock statement. The Dispose call or unlocking will happen on the original value of the local.

That warning is important - heed it.

Leaving that aside, it's entirely reasonable to put a try/catch block in a using statement... but in this case it should probably be outside the using statement, letting you initialize the response variable at the appropriate time so that the response will always be disposed. I would also consider using multiple catch blocks calling a common method rather than using "is" repeatedly.