Using for IDbConnection/IDbTransaction safe to use?

Neolisk picture Neolisk · Jun 3, 2013 · Viewed 12.5k times · Source

While my assumption may seem to sound subjective, after some research, I found that it's not uncommon to find developers who favour a dummy Try/Catch instead of using the Using statement for IDbConnection/IDbTransaction processing (Close/Commit/Rollback).

This holds true for even some of the most seasoned developers and some new ones. I am intentionally not going to reference any of the question on StackOverflow or forum links as an example, so people don't get offended. From what I found, Using statement is safe to use (no pun intended).

Is there anything wrong with it? Consider the following code:

Public Sub Commit()
  Dim cn As IDbConnection = {CREATE_CONNECTION}
  Dim tran As IDbTransaction = Nothing

  cn.Open()
  Try
    tran = cn.BeginTransaction
    'run some queries here
    tran.Commit()
  Catch ex As Exception
    If Not tran Is Nothing Then tran.Rollback()
    Throw
  Finally
    cn.Close()
  End Try
End Function

Assume {CREATE_CONNECTION} is place holder for a Sub that creates a connection, depending on the database vendor, written according to all possible best practices and does not need more improvement.

Is there a reason why the above code cannot be rewritten as such:

Using cn As IDbConnection = {CREATE_CONNECTION}
  cn.Open()
  Using tran As IDbTransaction = cn.BeginTransaction
    'run some queries here
    tran.Commit()
  End Using
End Using

?

Clearly, version #2 is more intuitive to what it's doing. But perhaps I am missing something important here? Things like vendor-specific implementations of data access libraries, that do not call Transaction.Commit and/or Connection.Close on Dispose internally? Is this approach being decommissioned in the near future, or not regarded as clear enough in modern programming pattern/best practices? Mono/mobile apps dev tools lacking debug support for Using keyword?

I am looking for any kind of answer to support or deny the point. Preferably the one with quotes to original documentation, something like Do not use Using with IDbTransaction when .... Links to blogs or personal experience are okay too.

Answer

Marc Gravell picture Marc Gravell · Jun 3, 2013

I'm entirely with you on the connection; that should be using, and there is no need for the explicit Close(). The transaction is a little bit trickier; the code shown is certainly overkill at the moment, but it is not entirely defined that Dispose() should do a rollback. Actually, that is what tends to happen in every implementation I've looked at, but it is slightly vexing that even DbTransaction (which most providers use) doesn't actually do this. Contrast to TransactionScope where it is explicitly defined that a Dispose() without a commit counts as a rollback. Because of that, I tend to use (excuse the C#):

using(var conn = GetOpenConnection())
using(var tran = conn.BeginTransaction()) {
    try {
        // TODO: do work
        tran.Commit();
    } catch {
        tran.Rollback();
        throw;
    }
}

which is somewhere between the two in terms of complexity. It isn't messing around with null-checks, at least.