ASP.NET Hosting

Correct exception handling

If you look at this post by SantoshZ of the C# team, you'll see code like this:

Connection conn = null;
try
{
  conn = new Connection();
  conn.Open();
}
catch
{
  if (conn != null) conn.Close();
}

Well, this is quite a strange code snippet coming from the C# team, I must say! It demonstrates bad exception handling.
Here is what this code should look like in my opinion:

Connection conn = new Connection();
conn.Open();
try
{
  ...
}
finally
{
  conn.Close();
}

Why is that?

  1. There is no need to call Close() if Open() was not previously executed.
  2. There is no need to call Close() if Open() failed.
  3. You probably want "finally" instead of "catch"

Too often do I see code all in a big try...catch block.

BTW, don't forget to use the using statement, which greatly simplifies code in a lot of cases.

20 Comments

  • Fabrice,

    What happens if conn.Open() throws.



    My personal take is that the code should be:



    Connection conn = null;

    try

    {

    conn = new Connection();

    }

    catch (<whatever>)

    {

    < handle new failure >

    }

    try

    {

    conn.Open();

    }

    catch (<open failure>)

    {

    < handle open failure >

    }

    try

    {

    ...

    }

    finally

    {

    conn.Close();

    }



    If you don't need to differentiate between the new and the open, you could group them together.

  • Frankly, I think Dispose() should be called in either case. Seems to me if you have an instance of an object which implements IDisposable, Dispose() should be called if you have an instance (ie, it should be called reguardless of whether Open() failed or not). This can be done, as you both mentioned, via using.



    Also, isn't santoshZ's code swallowing an Exception?



  • I vote for larrys version, your have to catch the connection opening error..



    Last finally would be:





    finally

    {

    if (conn != null) conn.Close();

    }

  • Actually the original code was better than yours. I don't have any supporting URLs for this, but calling .Close() on an open connection is 100% fine. I haven't run reflector, but I would be 98% sure it checks whether the connection is open before closing it. And if it doesn't, it at least doesn't cause any error. But he should definitely close the connection - finally IS better.



    Dispose() and Close() are equivalent for SqlConnection, so you can call either, but I tend for consistency to call Dispose().



    Larry has it better - there is a very strong chance that conn.open would throw an exception. I am a little lazy with exceptions - I don't have time to check the different kinds of exceptions that each statement could possibly throw, so I tend to wrap the whole of functions at the top of the calling stack in



    try {

    }

    catch (Exception ex) {

    // show an 'error occurred' message box

    }



    The only problem with this pattern is that you need to dispose of unmanaged resources, and if you declare the connection inside the try {} block, it will be out of scope. So go ahead and declare them SqlConnection cnn = null, but please don't dare open it outside of the safety of try{}.



    Larry's code is pretty safe, but if you have a more complex block, such as this:



    SqlConnection cnn = null;

    try {

    this.SomeFunctionThatCanRaiseException();

    cnn = new SqlConnection();

    }

    finally {

    cnn.Dispose();

    }



    cnn could be null.



    So the correct pattern is



    SqlConnection cnn = null;

    try {

    SqlConnection cnn = new SqlConnection();

    }

    catch (Exception ex) {

    // insert error handling code here

    }

    finally {

    if (cnn != null) cnn.Dispose(); // or close, but dispose is consistent with other unmanaged resources

    }

  • I had a look at some of my code, and here's another way to do it:



    SqlConnection connection = new SqlConnection();

    try {

    connection.connectionString = whatever;

    }

    catch (Exception ex) {

    }

    finally {

    connection.Close();

    }

  • I was right (on Close being absolutely fine on open connections).



    From reflector:



    public void Close()

    {

    SqlCommand command1;

    switch (this._objectState)

    {

    case ConnectionState.Closed:

    {

    return;

    }

    case ConnectionState.Open:

    {

    goto Label_0016;

    }

    }

    return;

    Label_0016:

    this.CloseReader();

    if (ConnectionState.Open != this._objectState)

    {

    return;

    }

  • Guys, the original code and this code is not about SqlConnection or even an IDbConnection. It is about a code pattern. So we don't care about what Close() really do. The point here is that it is useless to call Close() if Open() did not happen. And matthew just demonstrated this with a real case.

  • Larry, do you enclose every line of code with a try...catch? Looks like overkill to me.

    What happens if Open() throws an exception? Well we are safe because none of the code below the call to Open() is executed.

    Of course, you can use a try...catch to throw a more user friendly exception if this is what you want.

    But thanks to exceptions, not everything needs to be checked all the time. We are always safe. This is not the case with languages that don't support exception, in which case you have to check the result of every call to protect your program from going amuck.

    That's what exceptions really are: safe and simple.

  • Connection conn = null;

    try

    {

    conn = new Connection();

    conn.Open();

    conn.Close();

    }

    catch

    {

    ....

    }

    finally

    {

    conn.Close();

    }



    From what i know there no harm in calling conn.Close twice. Executing conn.Close in the try ensures early release of the resource. In case of exception the connection gets released in the finally block.



    The problem with the approach mentioned by you, is that the connection will remain open till the finally block is executed.



  • Amit, there is a problem with your code: if the creation of the Connection fails, conn stays null, but you try nonetheless to call Close() on it... kaboom!



    try...finally blocks are here specifically for that: cleaning up things when you want to. I don't see the point in calling Close() twice?! If you want to close the connection sooner, then make your try...finally block smaller, and continue your processing after the block.



    With your code, you're not only calling Close() twice, which is useless and confusing, but you specifically require that a call to Close() is made even if Open() never happens or fails!



    Why do you all guys want to perform unneeded processing?

  • Fabrice,

    I don't use exceptions in my code. Ever, if I have a choice. If I'm using a framework that throws (like ATL collections), I wrap every single line that might throw with a try/catch. Just like I assign an HRESULT variable to the result of every function call that might fail and check the error code.



  • Larry, why is that? It seems strange. What is the specific reason for doing so?

  • I'm seeing lots of "if the creation of the Connection object fails..." statements. What exceptions can the Connection constructor throw? Shouldn't you structure you statement like this?



    Connection con = null;

    try

    {

    conn = new Connection();

    conn.Open();

    conn.Close();

    }

    catch( CreateConnectionException ccex)

    {

    //handle connection creation exception

    }

    catch ( SqlException sqlEx)

    {

    //handle conn.Open connection

    }

    finally

    {

    if(conn != null)

    conn.Close();

    }





    I think that does the same thing as Larry's, but it's less confusing to me. Granted I haven't typed it in to see if it would compile<grin>.

  • Scott, in this case you don't need the first Close(), because you do it in the finally block.

    Do you always try to catch everything that COULD possibly happen? All your code must be crippled with try...catch...finally blocks, if you want to protect every constructor call.

  • Well we share the same religion then ;-) That's the way I do it too.



    I don't think I'd like checked exceptions. It seems too much work for me. Since I don't see a need for catching specific exceptions classes (except in rare occasions of course), I don't really need to know what comes out.

    But I don't want to join the big debate on checked exceptions. As you say, it's like argueing politics, and that's not what I started that post for :-)

  • For what it's worth and slightly (or very much) off topic, I like to think about exception handling in the following way: Each resource that needs protection should have its own try/finally block. So when you have grabbed the resource, you protect it with a try/finally. Pretty much as Fabrice said.

    :-)



    Best Regards,

    Jimmy

    www.jnsk.se/weblog/

    ###

  • fabrice, you sate that you don't catch every exception....



    So what happens? Your program crashes?



    Every line of code should be wrapped in try catch (except where you are happy to propogate the exception to the calliung function).

  • > So what happens? Your program crashes?



    Of course not. I don't catch every exception immediately. I catch them when it is useful. Only a few get handled locally in fact. So most of the time, the exceptions propagate to the calling functions.

    Ultimately, if an exception is not handled by any function, it gets stopped in a ThreadException handler (for Windows applications) or in Global.asax (for ASP.NET applications), logged, and displayed to the user.

  • I disagree with putting Open outside of the try. What if you cant connect to the server? Then you get an uncaught exception.

    A better way to do it is:

    try
    {
    if(connection.State == State.Closed)
    connection.Open()
    //do stuff
    }
    catch(SqlException se)
    {
    //log error, etc...
    }
    finally
    {
    if(connection.State == State.Open)
    connection.Close()
    }

    This way you're only opening or closing if its not already opened or closed, and you're catching exceptions properly.

  • Mac, in the original code, the connection is created right before calling Open. So, there is no possibility that the connection is closed when we call Open.
    In other cases, I agree that you should test for the connection state before calling opening it.

Comments have been disabled for this content.