22

In the below code if any exception is thrown while executing the the SQL statements we should expect an implicit rollback on the transaction as the transaction was not committed, it goes out of scope and it gets disposed:

using (DbTransaction tran = conn.BeginTransaction())
{
    //
    // Execute SQL statements here...
    //
    tran.Commit();
}

Is the above an acceptable practice, or should one catch the exception and explicitly make a call to tran.Rollback() as shown below:

using (DbTransaction tran = conn.BeginTransaction())
{
    try
    {
        //
        // Execute SQL statements here...
        //
        tran.Commit();
    }
    catch
    {
        tran.Rollback();
        throw;
    }
}
Elan
  • 6,084
  • 12
  • 64
  • 84

3 Answers3

24

Former. If you look up MSDN samples on similar topics, like TransactionScope, they all favor the implicit rollback. There are various reasons for that, but I'll just give you a very simple one: by the time you catch the exception, the transaction may had already rolled back. Many errors rollback the pending transaction and then they return control to the client, where the ADO.Net raises the CLR SqlException after the transaction was already rolled back on the server (1205 DEADLOCK is the typical example of such an error), so the explicit Rollback() call is, at best, a no-op, and at worse an error. The provider of the DbTransaction (eg. SqlTransaction) should know how to handle this case, eg. because there is explicit chat between the server and the client notifying of the fact that the transaction rolled back already, and the Dispose() method does the right thing.

A second reason is that transactions can be nested, but the semantics of ROLLBACK are that one rollback rolls back all transactions, so you only need to call it once (unlike Commit() which commits only the inner most transaction and has to be called paired up for each begin). Again, Dispose() does the right thing.

Update

The MSDN sample for SqlConnection.BeginTransaction() actually favors the second form and does an explicit Rollback() in the catch block. I suspect the technical writer simply intended to show in one single sample both Rollback() and Commit(), notice how he needed to add a second try/catch block around the Rollback to circumvent exactly some of the problems I mentioned originally.

deveton
  • 291
  • 4
  • 26
Remus Rusanu
  • 288,378
  • 40
  • 442
  • 569
  • 1
    Actually, the problem is that data providers might not implement it that way: "Dispose should rollback the transaction. However, the behavior of Dispose is provider specific, and should not replace calling Rollback." via [MSDN](http://msdn.microsoft.com/en-us/library/bf2cw321.aspx) – Andre Luus Nov 15 '12 at 13:04
  • That said, I prefer the implicit approach. – Andre Luus Nov 15 '12 at 13:06
  • @AndreLuus: Good catch. Notice though that the MSDN you linked has this text only for version 4.5, ie. at the time I wrote the answer this was not yet on MSDN. – Remus Rusanu Nov 15 '12 at 13:35
  • True. At least we have an official position from Microsoft now. – Andre Luus Nov 15 '12 at 14:56
  • If I had to use the `try-catch` to do some error logging in the `catch{}`, should I explicitly call `Rollback()`? – Michael Apr 10 '14 at 20:24
  • @RemusRusanu Thanks for adding "Update" section to your post. Some readers like myself could have been confused as to why MSDN tech writer then is using explicit `Rollback` in his/her MSDN example. – nam Nov 03 '15 at 03:59
3

You can go either way, the former being more concise, the latter being more intent revealing.

A caveat with the first approach would be that calling RollBack on disposal of transaction is dependent on the driver specific implementation. Hopefully almost all the .NET connectors do that. SqlTransaction does:

private void Dispose(bool disposing)
{
    Bid.PoolerTrace("<sc.SqlInteralTransaction.Dispose|RES|CPOOL> %d#, Disposing\n", this.ObjectID);
    if (disposing && (this._innerConnection != null))
    {
        this._disposing = true;
        this.Rollback();
    }
}

MySQL's: 

protected override void Dispose(bool disposing)
{
  if ((conn != null && conn.State == ConnectionState.Open || conn.SoftClosed) && open)
    Rollback();
  base.Dispose(disposing);
}

A caveat with second approach is it's not safe to call RollBack without another try-catch. This is explicitly stated in the documentation.

In short as to which is better: it depends on the driver, but it's typically better to go for the first, for the reasons mentioned by Remus.

Additionally see What happens to an uncommitted transaction when the connection is closed? for as to how connection disposal treat commits and rollbacks.

Community
  • 1
  • 1
nawfal
  • 70,104
  • 56
  • 326
  • 368
2

I tend to agree with "Implicit" rollback based on exception pathways. But, obviously, that depends on where you are in the stack and what you're trying to get done (i.e. is the DBTranscation class catching the exception and performing cleanup, or is it passively not "committing"?).

Here's a case where implicit handling makes sense (maybe):

static T WithTranaction<T>(this SqlConnection con, Func<T> do) {
    using (var txn = con.BeginTransaction()) {
        return do();
    }
}

But, if the API is different, the handling of commit might be, too (granted, this :

static T WithTranaction<T>(this SqlConnection con, Func<T> do, 
    Action<SqlTransaction> success = null, Action<SqlTransaction> failure = null) 
{
    using (var txn = con.BeginTransaction()) {
        try {
            T t = do();
            success(txn); // does it matter if the callback commits?
            return t;
        } catch (Exception e) {
            failure(txn); // does it matter if the callback rolls-back or commits?
            // throw new Exception("Doh!", e); // kills the transaction for certain
            // return default(T); // if not throwing, we need to do something (bogus)
        }
    }
}

I can't think of too many cases where explicitly rolling-back is the right approach except where there is a strict change control policy to be enforced. But then again, I'm a bit slow on the uptake.

Lee
  • 801
  • 1
  • 6
  • 20