Using for IDbConnection/IDbTransaction safe to use?

Question

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.

Solution 1

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.

Solution 2

What you're seeing is developers coding according to the documentation (a "Good Thing"). The base class DbTransaction (used for most data providers' transaction implementations) states clearly in its documentation:

Dispose should rollback the transaction. However, the behavior of Dispose is provider specific, and should not replace calling Rollback.

Comments

[Victor Zakharov]

That explains it, thanks. Definitely more clear than my version #1. Also +1 for GetOpenConnection, I was also leaning towards incorporating cn.Open() into some CreateAndOpenConnection, as it seems redundant to call every time.

[Michael Gunter]

Thanks. MS should definitely make it clearer on the actual interface.

[Erwin Mayer]

If the connection to the database is closed, will the transaction still get rolled back? I understand you are more worried about possible locking.